symfony · fabpot · May 2, 2024 · Apr 2, 2024
diff --git a/src/Symfony/Bridge/Doctrine/ArgumentResolver/EntityValueResolver.php b/src/Symfony/Bridge/Doctrine/ArgumentResolver/EntityValueResolver.php
@@ -60,9 +60,9 @@ public function resolve(Request $request, ArgumentMetadata $argument): array
                $message = sprintf(' The expression "%s" returned null.', $options->expr);
            }
        // find by identifier?
-        } elseif (false === $object = $this->find($manager, $request, $options, $argument->getName())) {
+        } elseif (false === $object = $this->find($manager, $request, $options, $argument)) {
            // find by criteria
-            if (!$criteria = $this->getCriteria($request, $options, $manager)) {
+            if (!$criteria = $this->getCriteria($request, $options, $manager, $argument)) {
                return [];
            }
            try {
@@ -94,13 +94,13 @@ private function getManager(?string $name, string $class): ?ObjectManager
        return $manager->getMetadataFactory()->isTransient($class) ? null : $manager;
    }

-    private function find(ObjectManager $manager, Request $request, MapEntity $options, string $name): false|object|null
+    private function find(ObjectManager $manager, Request $request, MapEntity $options, ArgumentMetadata $argument): false|object|null
    {
        if ($options->mapping || $options->exclude) {
            return false;
        }

-        $id = $this->getIdentifier($request, $options, $name);
+        $id = $this->getIdentifier($request, $options, $argument);
        if (false === $id || null === $id) {
            return $id;
        }
@@ -119,14 +119,14 @@ private function find(ObjectManager $manager, Request $request, MapEntity $optio
        }
    }

-    private function getIdentifier(Request $request, MapEntity $options, string $name): mixed
+    private function getIdentifier(Request $request, MapEntity $options, ArgumentMetadata $argument): mixed
    {
        if (\is_array($options->id)) {
            $id = [];
            foreach ($options->id as $field) {
                // Convert "%s_uuid" to "foobar_uuid"
                if (str_contains($field, '%s')) {
-                    $field = sprintf($field, $name);
+                    $field = sprintf($field, $argument->getName());
                }

                $id[$field] = $request->attributes->get($field);
@@ -135,28 +135,54 @@ private function getIdentifier(Request $request, MapEntity $options, string $nam
            return $id;
        }

-        if (null !== $options->id) {
-            $name = $options->id;
+        if ($options->id) {
+            return $request->attributes->get($options->id) ?? ($options->stripNull ? false : null);
        }

+        $name = $argument->getName();
+
        if ($request->attributes->has($name)) {
-            return $request->attributes->get($name) ?? ($options->stripNull ? false : null);
+            if (\is_array($id = $request->attributes->get($name))) {
+                return false;
+            }
+
+            foreach ($request->attributes->get('_route_mapping') ?? [] as $parameter => $attribute) {
+                if ($name === $attribute) {
+                    $options->mapping = [$name => $parameter];
+
+                    return false;
+                }
+            }
+
+            return $id ?? ($options->stripNull ? false : null);
        }
+        if ($request->attributes->has('id')) {
+            trigger_deprecation('symfony/doctrine-bridge', '7.2', 'Relying on auto-mapping for Doctrine entities is deprecated for argument $%s of "%s": declare the mapping using either the #[MapEntity] attribute or mapped route parameters.', $argument->getName(), $argument->getControllerName());

-        if (!$options->id && $request->attributes->has('id')) {
            return $request->attributes->get('id') ?? ($options->stripNull ? false : null);
        }

        return false;
    }

-    private function getCriteria(Request $request, MapEntity $options, ObjectManager $manager): array
+    private function getCriteria(Request $request, MapEntity $options, ObjectManager $manager, ArgumentMetadata $argument): array
    {
-        if (null === $mapping = $options->mapping) {
+        if (!($mapping = $options->mapping) && \is_array($criteria = $request->attributes->get($argument->getName()))) {
+            foreach ($options->exclude as $exclude) {
+                unset($criteria[$exclude]);
+            }
+
+            if ($options->stripNull) {
+                $criteria = array_filter($criteria, static fn ($value) => null !== $value);
+            }
+
+            return $criteria;
+        } elseif (null === $mapping) {
+            trigger_deprecation('symfony/doctrine-bridge', '7.2', 'Relying on auto-mapping for Doctrine entities is deprecated for argument $%s of "%s": declare the identifier using either the #[MapEntity] attribute or mapped route parameters.', $argument->getName(), $argument->getControllerName());
            $mapping = $request->attributes->keys();
        }

-        if ($mapping && \is_array($mapping) && array_is_list($mapping)) {
+        if ($mapping && array_is_list($mapping)) {
            $mapping = array_combine($mapping, $mapping);
        }

@@ -168,17 +194,11 @@ private function getCriteria(Request $request, MapEntity $options, ObjectManager
            return [];
        }

-        // if a specific id has been defined in the options and there is no corresponding attribute
-        // return false in order to avoid a fallback to the id which might be of another object
-        if (\is_string($options->id) && null === $request->attributes->get($options->id)) {
-            return [];
-        }
-
        $criteria = [];
-        $metadata = $manager->getClassMetadata($options->class);
+        $metadata = null === $options->mapping ? $manager->getClassMetadata($options->class) : false;

        foreach ($mapping as $attribute => $field) {
-            if (!$metadata->hasField($field) && (!$metadata->hasAssociation($field) || !$metadata->isSingleValuedAssociation($field))) {
+            if ($metadata && !$metadata->hasField($field) && (!$metadata->hasAssociation($field) || !$metadata->isSingleValuedAssociation($field))) {
                continue;
            }


diff --git a/src/Symfony/Bridge/Doctrine/Attribute/MapEntity.php b/src/Symfony/Bridge/Doctrine/Attribute/MapEntity.php
@@ -47,6 +47,7 @@ public function __construct(
        public ?string $message = null,
    ) {
        parent::__construct($resolver, $disabled);
+        $this->selfValidate();
    }

    public function withDefaults(self $defaults, ?string $class): static
@@ -62,6 +63,22 @@ public function withDefaults(self $defaults, ?string $class): static
        $clone->evictCache ??= $defaults->evictCache ?? false;
        $clone->message ??= $defaults->message;

+        $clone->selfValidate();
+
        return $clone;
    }
+
+    private function selfValidate(): void
+    {
+        if (!$this->id) {
+            return;
+        }
+        if ($this->mapping) {
+            throw new \LogicException('The "id" and "mapping" options cannot be used together on #[MapEntity] attributes.');
+        }
+        if ($this->exclude) {
+            throw new \LogicException('The "id" and "exclude" options cannot be used together on #[MapEntity] attributes.');
+        }
+        $this->mapping = [];
+    }
 }
diff --git a/src/Symfony/Bridge/Doctrine/CHANGELOG.md b/src/Symfony/Bridge/Doctrine/CHANGELOG.md
@@ -8,6 +8,7 @@ CHANGELOG
 * Allow `EntityValueResolver` to return a list of entities
 * Add support for auto-closing idle connections
 * Allow validating every class against `UniqueEntity` constraint
+ * Deprecate auto-mapping of entities in favor of mapped route parameters

 7.0
 ---

diff --git a/src/Symfony/Bridge/Doctrine/Tests/ArgumentResolver/EntityValueResolverTest.php b/src/Symfony/Bridge/Doctrine/Tests/ArgumentResolver/EntityValueResolverTest.php
@@ -63,6 +63,9 @@ public function testResolveWithoutManager()
        $this->assertSame([], $resolver->resolve($request, $argument));
    }

+    /**
+     * @group legacy
+     */
    public function testResolveWithNoIdAndDataOptional()
    {
        $manager = $this->getMockBuilder(ObjectManager::class)->getMock();
@@ -83,18 +86,10 @@ public function testResolveWithStripNulls()

        $request = new Request();
        $request->attributes->set('arg', null);
-        $argument = $this->createArgument('stdClass', new MapEntity(stripNull: true), 'arg', true);
+        $argument = $this->createArgument('stdClass', new MapEntity(mapping: ['arg'], stripNull: true), 'arg', true);

-        $metadata = $this->getMockBuilder(ClassMetadata::class)->getMock();
-        $metadata->expects($this->once())
-            ->method('hasField')
-            ->with('arg')
-            ->willReturn(true);
-
-        $manager->expects($this->once())
-            ->method('getClassMetadata')
-            ->with('stdClass')
-            ->willReturn($metadata);
+        $manager->expects($this->never())
+            ->method('getClassMetadata');

        $manager->expects($this->never())
            ->method('getRepository');
@@ -139,7 +134,7 @@ public function testResolveWithNullId()
        $request = new Request();
        $request->attributes->set('id', null);

-        $argument = $this->createArgument(isNullable: true);
+        $argument = $this->createArgument(isNullable: true, entity: new MapEntity(id: 'id'));

        $this->assertSame([null], $resolver->resolve($request, $argument));
    }
@@ -195,6 +190,9 @@ public static function idsProvider(): iterable
        yield ['foo'];
    }

+    /**
+     * @group legacy
+     */
    public function testResolveGuessOptional()
    {
        $manager = $this->getMockBuilder(ObjectManager::class)->getMock();
@@ -232,16 +230,8 @@ public function testResolveWithMappingAndExclude()
            new MapEntity(mapping: ['foo' => 'Foo'], exclude: ['bar'])
        );

-        $metadata = $this->getMockBuilder(ClassMetadata::class)->getMock();
-        $metadata->expects($this->once())
-            ->method('hasField')
-            ->with('Foo')
-            ->willReturn(true);
-
-        $manager->expects($this->once())
-            ->method('getClassMetadata')
-            ->with('stdClass')
-            ->willReturn($metadata);
+        $manager->expects($this->never())
+            ->method('getClassMetadata');

        $repository = $this->getMockBuilder(ObjectRepository::class)->getMock();
        $repository->expects($this->once())
@@ -257,6 +247,42 @@ public function testResolveWithMappingAndExclude()
        $this->assertSame([$object], $resolver->resolve($request, $argument));
    }

+    public function testResolveWithRouteMapping()
+    {
+        $manager = $this->getMockBuilder(ObjectManager::class)->getMock();
+        $registry = $this->createRegistry($manager);
+        $resolver = new EntityValueResolver($registry);
+
+        $request = new Request();
+        $request->attributes->set('conference', 'vienna-2024');
+        $request->attributes->set('article', ['title' => 'foo']);
+        $request->attributes->set('_route_mapping', ['slug' => 'conference']);
+
+        $argument1 = $this->createArgument('Conference', new MapEntity('Conference'), 'conference');
+        $argument2 = $this->createArgument('Article', new MapEntity('Article'), 'article');
+
+        $manager->expects($this->never())
+            ->method('getClassMetadata');
+
+        $conference = new \stdClass();
+        $article = new \stdClass();
+
+        $repository = $this->getMockBuilder(ObjectRepository::class)->getMock();
+        $repository->expects($this->any())
+            ->method('findOneBy')
+            ->willReturnCallback(static fn ($v) => match ($v) {
+                ['slug' => 'vienna-2024'] => $conference,
+                ['title' => 'foo'] => $article,
+            });
+
+        $manager->expects($this->any())
+            ->method('getRepository')
+            ->willReturn($repository);
+
+        $this->assertSame([$conference], $resolver->resolve($request, $argument1));
+        $this->assertSame([$article], $resolver->resolve($request, $argument2));
+    }
+
    public function testExceptionWithExpressionIfNoLanguageAvailable()
    {
        $manager = $this->getMockBuilder(ObjectManager::class)->getMock();

diff --git a/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php b/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php
@@ -60,7 +60,7 @@ public function getArguments(Request $request, callable $controller, ?\Reflectio
                    if ($attribute->disabled) {
                        $disabledResolvers[$attribute->resolver] = true;
                    } elseif ($resolverName) {
-                        throw new \LogicException(sprintf('You can only pin one resolver per argument, but argument "$%s" of "%s()" has more.', $metadata->getName(), $this->getPrettyName($controller)));
+                        throw new \LogicException(sprintf('You can only pin one resolver per argument, but argument "$%s" of "%s()" has more.', $metadata->getName(), $metadata->getControllerName()));
                    } else {
                        $resolverName = $attribute->resolver;
                    }
@@ -118,7 +118,7 @@ public function getArguments(Request $request, callable $controller, ?\Reflectio
                }
            }

-            throw new \RuntimeException(sprintf('Controller "%s" requires the "$%s" argument that could not be resolved. '.($reasonCounter > 1 ? 'Possible reasons: ' : '').'%s', $this->getPrettyName($controller), $metadata->getName(), implode(' ', $reasons)));
+            throw new \RuntimeException(sprintf('Controller "%s" requires the "$%s" argument that could not be resolved. '.($reasonCounter > 1 ? 'Possible reasons: ' : '').'%s', $metadata->getControllerName(), $metadata->getName(), implode(' ', $reasons)));
        }

        return $arguments;
@@ -137,21 +137,4 @@ public static function getDefaultArgumentValueResolvers(): iterable
            new VariadicValueResolver(),
        ];
    }
-
-    private function getPrettyName($controller): string
-    {
-        if (\is_array($controller)) {
-            if (\is_object($controller[0])) {
-                $controller[0] = get_debug_type($controller[0]);
-            }
-
-            return $controller[0].'::'.$controller[1];
-        }
-
-        if (\is_object($controller)) {
-            return get_debug_type($controller);
-        }
-
-        return $controller;
-    }
 }
diff --git a/src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadata.php b/src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadata.php
@@ -31,6 +31,7 @@ public function __construct(
        private mixed $defaultValue,
        private bool $isNullable = false,
        private array $attributes = [],
+        private string $controllerName = 'n/a',
    ) {
        $this->isNullable = $isNullable || null === $type || ($hasDefaultValue && null === $defaultValue);
    }
@@ -135,4 +136,9 @@ public function getAttributesOfType(string $name, int $flags = 0): array

        return $attributes;
    }
+
+    public function getControllerName(): string
+    {
+        return $this->controllerName;
+    }
 }
diff --git a/src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadataFactory.php b/src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadataFactory.php
@@ -22,6 +22,7 @@
    {
        $arguments = [];
        $reflector ??= new \ReflectionFunction($controller(...));
+        $controllerName = $this->getPrettyName($reflector);

        foreach ($reflector->getParameters() as $param) {
            $attributes = [];
@@ -31,7 +32,7 @@
                }
            }

-            $arguments[] = new ArgumentMetadata($param->getName(), $this->getType($param), $param->isVariadic(), $param->isDefaultValueAvailable(), $param->isDefaultValueAvailable() ? $param->getDefaultValue() : null, $param->allowsNull(), $attributes);
+            $arguments[] = new ArgumentMetadata($param->getName(), $this->getType($param), $param->isVariadic(), $param->isDefaultValueAvailable(), $param->isDefaultValueAvailable() ? $param->getDefaultValue() : null, $param->allowsNull(), $attributes, $controllerName);
        }

        return $arguments;
@@ -53,4 +54,19 @@
            default => $name,
        };
    }
+
+    private function getPrettyName(\ReflectionFunctionAbstract $r): string
+    {
+        $name = $r->name;
+
+        if ($r instanceof \ReflectionMethod) {
+            return $r->class.'::'.$name;
+        }
+
+        if ($r->isAnonymous() || !$class = $r->getClosureCalledClass()) {
+            return $name;
+        }
+
+        return $class->name.'::'.$name;
+    }
 }
diff --git a/src/Symfony/Component/HttpKernel/Tests/Controller/ArgumentResolverTest.php b/src/Symfony/Component/HttpKernel/Tests/Controller/ArgumentResolverTest.php
@@ -184,7 +184,7 @@ public function testIfExceptionIsThrownWhenMissingAnArgument()
        $controller = (new ArgumentResolverTestController())->controllerWithFoo(...);

        $this->expectException(\RuntimeException::class);
-        $this->expectExceptionMessage('Controller "Closure" requires the "$foo" argument that could not be resolved. Either the argument is nullable and no null value has been provided, no default value has been provided or there is a non-optional argument after this one.');
+        $this->expectExceptionMessage('Controller "'.ArgumentResolverTestController::class.'::controllerWithFoo" requires the "$foo" argument that could not be resolved. Either the argument is nullable and no null value has been provided, no default value has been provided or there is a non-optional argument after this one.');
        self::getResolver()->getArguments($request, $controller);
    }