Skip to content

[DependencyInjection] Fix support for unions/intersections together with ServiceSubscriberInterface #43930

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 6, 2021

Conversation

kbond
Copy link
Member

@kbond kbond commented Nov 4, 2021

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

Continuation of #43479 per discussion with @nicolas-grekas.

Todo:

  • intersection type support/tests
  • ServiceSubscriberTrait support

@carsonbot carsonbot added this to the 5.4 milestone Nov 4, 2021
@carsonbot carsonbot changed the title [WIP][DependencyInjection] Enable union/intersect services in ServiceSubscriberInterface [DependencyInjection] [WIP] Enable union/intersect services in ServiceSubscriberInterface Nov 4, 2021
Copy link
Member

@nicolas-grekas 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.

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

@kbond kbond force-pushed the service-subscriber-union branch from 662c282 to 81cd56f Compare November 4, 2021 16:00
@kbond
Copy link
Member Author

kbond commented Nov 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.

@nicolas-grekas
Copy link
Member

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()) {

@nicolas-grekas
Copy link
Member

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

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

Adding str_replace(['|', '&'], ['|\\', '&\\'], ...) around the call to getType() should do it.

@kbond
Copy link
Member Author

kbond commented Nov 5, 2021

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

Copy link
Member

@nicolas-grekas 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?

@nicolas-grekas nicolas-grekas changed the title [DependencyInjection] [WIP] Enable union/intersect services in ServiceSubscriberInterface [DependencyInjection] Fix support for unions/intersections together with ServiceSubscriberInterface Nov 6, 2021
@nicolas-grekas nicolas-grekas added Bug and removed Feature labels Nov 6, 2021
@nicolas-grekas nicolas-grekas force-pushed the service-subscriber-union branch from 48b3262 to 123842a Compare November 6, 2021 10:24
@nicolas-grekas
Copy link
Member

Thank you @kbond.

@nicolas-grekas nicolas-grekas merged commit 63f5e27 into symfony:5.4 Nov 6, 2021
@kbond kbond deleted the service-subscriber-union branch November 6, 2021 12:35
This was referenced Nov 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants