Movatterモバイル変換

carsonbot added this to the5.4 milestone

carsonbot changed the title~~[WIP][DependencyInjection] Enable union/intersect services inServiceSubscriberInterface~~[DependencyInjection] [WIP] Enable union/intersect services inServiceSubscriberInterface

kbond commented

src/Symfony/Component/DependencyInjection/Compiler/RegisterServiceSubscribersPass.phpShow resolvedHide resolved

kbond commented

src/Symfony/Component/DependencyInjection/Tests/Compiler/RegisterServiceSubscribersPassTest.phpShow resolvedHide resolved

nicolas-grekas reviewed

symfony/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

Copy link

Member

nicolas-grekas left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think we'll also need to filter out any built-in types from union types, and to order the list of types for both union and intersection types.
We could do this in AutowirePass, when TypedReference are wired

src/Symfony/Component/DependencyInjection/Compiler/RegisterServiceSubscribersPass.phpShow resolvedHide resolved

kbond force-pushed theservice-subscriber-union branch from662c282 to81cd56fCompare

November 4, 2021 16:00

Copy link

MemberAuthor

kbond commentedNov 5, 2021

I think we'll also need to filter out any built-in types from union types, and to order the list of types for both union and intersection types.
We could do this in AutowirePass, when TypedReference are wired

Sorry, I'm quite out of my element here. I'm not sure how/where to do this.

Copy link

Member

nicolas-grekas commentedNov 5, 2021

Using pseudo-code, this is what is missing to me:

--- a/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php+++ b/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php@@ -100,7 +100,7 @@ class AutowirePass extends AbstractRecursivePass     private function doProcessValue(mixed $value, bool $isRoot = false): mixed     {         if ($value instanceof TypedReference) {-            if ($ref = $this->getAutowiredReference($value)) {+            if ($ref = $this->getAutowiredReference($value, true)) {                 return $ref;             }             if (ContainerBuilder::RUNTIME_EXCEPTION_ON_INVALID_REFERENCE === $value->getInvalidBehavior()) {@@ -291,7 +291,7 @@ class AutowirePass extends AbstractRecursivePass             }              $getValue = function () use ($type, $parameter, $class, $method) {-                if (!$value = $this->getAutowiredReference($ref = new TypedReference($type, $type, ContainerBuilder::EXCEPTION_ON_INVALID_REFERENCE, Target::parseName($parameter)))) {+                if (!$value = $this->getAutowiredReference($ref = new TypedReference($type, $type, ContainerBuilder::EXCEPTION_ON_INVALID_REFERENCE, Target::parseName($parameter)), false)) {                     $failureMessage = $this->createTypeNotFoundMessageCallback($ref, sprintf('argument "$%s" of method "%s()"', $parameter->name, $class !== $this->currentId ? $class.'::'.$method : $method));                      if ($parameter->isDefaultValueAvailable()) {@@ -346,7 +346,7 @@ class AutowirePass extends AbstractRecursivePass     /**      * Returns a reference to the service matching the given type, if any.      */-    private function getAutowiredReference(TypedReference $reference): ?TypedReference+    private function getAutowiredReference(TypedReference $reference, bool $filterType): ?TypedReference     {         $this->lastFailure = null;         $type = $reference->getType();@@ -355,6 +355,13 @@ class AutowirePass extends AbstractRecursivePass             return $reference;         }+        if ($filterType) {+            if (is_union_type($type)) {+                $type = remove_builtin_types($types);+            }+            $types = sort_types_in_union_or_intersection($types);+        }+         if (null !== $name = $reference->getName()) {

Copy link

Member

nicolas-grekas commentedNov 5, 2021

And we'll also need a change on this line, since the leading\ will be missing in union/intersect lists:

Line 1757 ind7ea3de

$returnedType =sprintf(': %s\%s', ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE >=$value->getInvalidBehavior() ?'' :'?',$value->getType());

Addingstr_replace(['|', '&'], ['|\\', '&\\'], ...) around the call togetType() should do it.

kbond requested a review fromdunglas as acode owner

November 5, 2021 13:14

carsonbot added Status: Needs Review and removed Status: Needs Work labels

kbond force-pushed theservice-subscriber-union branch fromd66eb54 toe4f1fddCompare

November 5, 2021 13:15

Copy link

MemberAuthor

kbond commentedNov 5, 2021

Thank you, I've added the requested code. Still not sure how to test this.

nicolas-grekas reviewed

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

thanks, we're almost done :)
can you try adding a few test cases that involve AutowirePass?

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php OutdatedShow resolvedHide resolved

kbond commented

src/Symfony/Component/DependencyInjection/Tests/Compiler/RegisterServiceSubscribersPassTest.php OutdatedShow resolvedHide resolved

kbond commented

src/Symfony/Component/DependencyInjection/Tests/Compiler/RegisterServiceSubscribersPassTest.phpShow resolvedHide resolved

nicolas-grekas approved these changes

src/Symfony/Component/DependencyInjection/Tests/Compiler/RegisterServiceSubscribersPassTest.phpShow resolvedHide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels

nicolas-grekas changed the title~~[DependencyInjection] [WIP] Enable union/intersect services inServiceSubscriberInterface~~[DependencyInjection] Fix support for unions/intersections together withServiceSubscriberInterface

nicolas-grekas added Bug and removed Feature labels