Skip to content

Navigation Menu

Sign in

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit b2e6e49

Browse filesBrowse files
fabpotfabpot
committed
feature #54455 [DoctrineBridge] Deprecate auto-mapping of entities in favor of mapped route parameters (nicolas-grekas)
This PR was merged into the 7.1 branch. Discussion ---------- [DoctrineBridge] Deprecate auto-mapping of entities in favor of mapped route parameters | Q | A | ------------- | --- | Branch? | 7.1 | Bug fix? | no | New feature? | yes | Deprecations? | | Issues | Fix #50739 | License | MIT Auto-mapping of entities on controllers is a foot-gun when multiple entities are listed on the signature of the controller. This is described extensively by e.g. `@stof` in the linked issue and in a few others. The issue is that we try to use all request attributes to call a `findOneBy`, but when there are many entities, there can be an overlap in the naming of the field/associations of both entities. In this PR, I propose to deprecate auto-mapping and to replace it with mapped route parameters, as introduced in #54720. If we go this way, people that use auto-mapping to e.g. load a `$conference` based on its `{slug}` will have to declare the mapping by using `{slug:conference}` instead. That makes everything explicit and keeps a nice DX IMHO, by not forcing a `#[MapEntity]` attribute for simple cases. Commits ------- a49f9ea [DoctrineBridge] Deprecate auto-mapping of entities in favor of mapped route parameters
2 parents 66faca6 + a49f9ea commit b2e6e49
Copy full SHA for b2e6e49

File tree

9 files changed

+162
-93
lines changed
Filter options

9 files changed

+162
-93
lines changed

‎src/Symfony/Bridge/Doctrine/ArgumentResolver/EntityValueResolver.php

Copy file name to clipboardExpand all lines: src/Symfony/Bridge/Doctrine/ArgumentResolver/EntityValueResolver.php
+41-21
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,9 @@ public function resolve(Request $request, ArgumentMetadata $argument): array
6060
$message = sprintf(' The expression "%s" returned null.', $options->expr);
6161
}
6262
// find by identifier?
63-
} elseif (false === $object = $this->find($manager, $request, $options, $argument->getName())) {
63+
} elseif (false === $object = $this->find($manager, $request, $options, $argument)) {
6464
// find by criteria
65-
if (!$criteria = $this->getCriteria($request, $options, $manager)) {
65+
if (!$criteria = $this->getCriteria($request, $options, $manager, $argument)) {
6666
return [];
6767
}
6868
try {
@@ -94,13 +94,13 @@ private function getManager(?string $name, string $class): ?ObjectManager
9494
return $manager->getMetadataFactory()->isTransient($class) ? null : $manager;
9595
}
9696

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

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

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

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

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

142+
$name = $argument->getName();
143+
142144
if ($request->attributes->has($name)) {
143-
return $request->attributes->get($name) ?? ($options->stripNull ? false : null);
145+
if (\is_array($id = $request->attributes->get($name))) {
146+
return false;
147+
}
148+
149+
foreach ($request->attributes->get('_route_mapping') ?? [] as $parameter => $attribute) {
150+
if ($name === $attribute) {
151+
$options->mapping = [$name => $parameter];
152+
153+
return false;
154+
}
155+
}
156+
157+
return $id ?? ($options->stripNull ? false : null);
144158
}
159+
if ($request->attributes->has('id')) {
160+
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());
145161

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

150165
return false;
151166
}
152167

153-
private function getCriteria(Request $request, MapEntity $options, ObjectManager $manager): array
168+
private function getCriteria(Request $request, MapEntity $options, ObjectManager $manager, ArgumentMetadata $argument): array
154169
{
155-
if (null === $mapping = $options->mapping) {
170+
if (!($mapping = $options->mapping) && \is_array($criteria = $request->attributes->get($argument->getName()))) {
171+
foreach ($options->exclude as $exclude) {
172+
unset($criteria[$exclude]);
173+
}
174+
175+
if ($options->stripNull) {
176+
$criteria = array_filter($criteria, static fn ($value) => null !== $value);
177+
}
178+
179+
return $criteria;
180+
} elseif (null === $mapping) {
181+
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());
156182
$mapping = $request->attributes->keys();
157183
}
158184

159-
if ($mapping && \is_array($mapping) && array_is_list($mapping)) {
185+
if ($mapping && array_is_list($mapping)) {
160186
$mapping = array_combine($mapping, $mapping);
161187
}
162188

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

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

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

‎src/Symfony/Bridge/Doctrine/Attribute/MapEntity.php

Copy file name to clipboardExpand all lines: src/Symfony/Bridge/Doctrine/Attribute/MapEntity.php
+17
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ public function __construct(
4747
public ?string $message = null,
4848
) {
4949
parent::__construct($resolver, $disabled);
50+
$this->selfValidate();
5051
}
5152

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

66+
$clone->selfValidate();
67+
6568
return $clone;
6669
}
70+
71+
private function selfValidate(): void
72+
{
73+
if (!$this->id) {
74+
return;
75+
}
76+
if ($this->mapping) {
77+
throw new \LogicException('The "id" and "mapping" options cannot be used together on #[MapEntity] attributes.');
78+
}
79+
if ($this->exclude) {
80+
throw new \LogicException('The "id" and "exclude" options cannot be used together on #[MapEntity] attributes.');
81+
}
82+
$this->mapping = [];
83+
}
6784
}

‎src/Symfony/Bridge/Doctrine/CHANGELOG.md

Copy file name to clipboardExpand all lines: src/Symfony/Bridge/Doctrine/CHANGELOG.md
+1
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ CHANGELOG
88
* Allow `EntityValueResolver` to return a list of entities
99
* Add support for auto-closing idle connections
1010
* Allow validating every class against `UniqueEntity` constraint
11+
* Deprecate auto-mapping of entities in favor of mapped route parameters
1112

1213
7.0
1314
---

‎src/Symfony/Bridge/Doctrine/Tests/ArgumentResolver/EntityValueResolverTest.php

Copy file name to clipboardExpand all lines: src/Symfony/Bridge/Doctrine/Tests/ArgumentResolver/EntityValueResolverTest.php
+48-22
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,9 @@ public function testResolveWithoutManager()
6363
$this->assertSame([], $resolver->resolve($request, $argument));
6464
}
6565

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

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

88-
$metadata = $this->getMockBuilder(ClassMetadata::class)->getMock();
89-
$metadata->expects($this->once())
90-
->method('hasField')
91-
->with('arg')
92-
->willReturn(true);
93-
94-
$manager->expects($this->once())
95-
->method('getClassMetadata')
96-
->with('stdClass')
97-
->willReturn($metadata);
91+
$manager->expects($this->never())
92+
->method('getClassMetadata');
9893

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

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

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

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

235-
$metadata = $this->getMockBuilder(ClassMetadata::class)->getMock();
236-
$metadata->expects($this->once())
237-
->method('hasField')
238-
->with('Foo')
239-
->willReturn(true);
240-
241-
$manager->expects($this->once())
242-
->method('getClassMetadata')
243-
->with('stdClass')
244-
->willReturn($metadata);
233+
$manager->expects($this->never())
234+
->method('getClassMetadata');
245235

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

250+
public function testResolveWithRouteMapping()
251+
{
252+
$manager = $this->getMockBuilder(ObjectManager::class)->getMock();
253+
$registry = $this->createRegistry($manager);
254+
$resolver = new EntityValueResolver($registry);
255+
256+
$request = new Request();
257+
$request->attributes->set('conference', 'vienna-2024');
258+
$request->attributes->set('article', ['title' => 'foo']);
259+
$request->attributes->set('_route_mapping', ['slug' => 'conference']);
260+
261+
$argument1 = $this->createArgument('Conference', new MapEntity('Conference'), 'conference');
262+
$argument2 = $this->createArgument('Article', new MapEntity('Article'), 'article');
263+
264+
$manager->expects($this->never())
265+
->method('getClassMetadata');
266+
267+
$conference = new \stdClass();
268+
$article = new \stdClass();
269+
270+
$repository = $this->getMockBuilder(ObjectRepository::class)->getMock();
271+
$repository->expects($this->any())
272+
->method('findOneBy')
273+
->willReturnCallback(static fn ($v) => match ($v) {
274+
['slug' => 'vienna-2024'] => $conference,
275+
['title' => 'foo'] => $article,
276+
});
277+
278+
$manager->expects($this->any())
279+
->method('getRepository')
280+
->willReturn($repository);
281+
282+
$this->assertSame([$conference], $resolver->resolve($request, $argument1));
283+
$this->assertSame([$article], $resolver->resolve($request, $argument2));
284+
}
285+
260286
public function testExceptionWithExpressionIfNoLanguageAvailable()
261287
{
262288
$manager = $this->getMockBuilder(ObjectManager::class)->getMock();

‎src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php
+2-19
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ public function getArguments(Request $request, callable $controller, ?\Reflectio
6060
if ($attribute->disabled) {
6161
$disabledResolvers[$attribute->resolver] = true;
6262
} elseif ($resolverName) {
63-
throw new \LogicException(sprintf('You can only pin one resolver per argument, but argument "$%s" of "%s()" has more.', $metadata->getName(), $this->getPrettyName($controller)));
63+
throw new \LogicException(sprintf('You can only pin one resolver per argument, but argument "$%s" of "%s()" has more.', $metadata->getName(), $metadata->getControllerName()));
6464
} else {
6565
$resolverName = $attribute->resolver;
6666
}
@@ -118,7 +118,7 @@ public function getArguments(Request $request, callable $controller, ?\Reflectio
118118
}
119119
}
120120

121-
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)));
121+
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)));
122122
}
123123

124124
return $arguments;
@@ -137,21 +137,4 @@ public static function getDefaultArgumentValueResolvers(): iterable
137137
new VariadicValueResolver(),
138138
];
139139
}
140-
141-
private function getPrettyName($controller): string
142-
{
143-
if (\is_array($controller)) {
144-
if (\is_object($controller[0])) {
145-
$controller[0] = get_debug_type($controller[0]);
146-
}
147-
148-
return $controller[0].'::'.$controller[1];
149-
}
150-
151-
if (\is_object($controller)) {
152-
return get_debug_type($controller);
153-
}
154-
155-
return $controller;
156-
}
157140
}

‎src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadata.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadata.php
+6
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ public function __construct(
3131
private mixed $defaultValue,
3232
private bool $isNullable = false,
3333
private array $attributes = [],
34+
private string $controllerName = 'n/a',
3435
) {
3536
$this->isNullable = $isNullable || null === $type || ($hasDefaultValue && null === $defaultValue);
3637
}
@@ -135,4 +136,9 @@ public function getAttributesOfType(string $name, int $flags = 0): array
135136

136137
return $attributes;
137138
}
139+
140+
public function getControllerName(): string
141+
{
142+
return $this->controllerName;
143+
}
138144
}

‎src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadataFactory.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadataFactory.php
+17-1
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ public function createArgumentMetadata(string|object|array $controller, ?\Reflec
2222
{
2323
$arguments = [];
2424
$reflector ??= new \ReflectionFunction($controller(...));
25+
$controllerName = $this->getPrettyName($reflector);
2526

2627
foreach ($reflector->getParameters() as $param) {
2728
$attributes = [];
@@ -31,7 +32,7 @@ public function createArgumentMetadata(string|object|array $controller, ?\Reflec
3132
}
3233
}
3334

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

3738
return $arguments;
@@ -53,4 +54,19 @@ private function getType(\ReflectionParameter $parameter): ?string
5354
default => $name,
5455
};
5556
}
57+
58+
private function getPrettyName(\ReflectionFunctionAbstract $r): string
59+
{
60+
$name = $r->name;
61+
62+
if ($r instanceof \ReflectionMethod) {
63+
return $r->class.'::'.$name;
64+
}
65+
66+
if ($r->isAnonymous() || !$class = $r->getClosureCalledClass()) {
67+
return $name;
68+
}
69+
70+
return $class->name.'::'.$name;
71+
}
5672
}

‎src/Symfony/Component/HttpKernel/Tests/Controller/ArgumentResolverTest.php

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/Tests/Controller/ArgumentResolverTest.php
+1-1
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ public function testIfExceptionIsThrownWhenMissingAnArgument()
184184
$controller = (new ArgumentResolverTestController())->controllerWithFoo(...);
185185

186186
$this->expectException(\RuntimeException::class);
187-
$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.');
187+
$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.');
188188
self::getResolver()->getArguments($request, $controller);
189189
}
190190

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.