8000 feature #54455 [DoctrineBridge] Deprecate auto-mapping of entities in… · symfony/symfony@b2e6e49 · GitHub
[go: up one dir, main page]
Skip to content

Commit b2e6e49

Browse 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

File tree

9 files changed

+162
-93
lines changed

9 files changed

+162
-93
lines changed

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

Lines changed: 41 additions & 21 deletions
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, MapE F438 ntity $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 === F438 $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

Lines changed: 17 additions & 0 deletions
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

Lines changed: 1 addition & 0 deletions
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

Lines changed: 48 additions & 22 deletions
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
< D7AE span class=pl-c1>$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

Lines changed: 2 additions & 19 deletions
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

Lines changed: 6 additions & 0 deletions
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

Lines changed: 17 additions & 1 deletion
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

Lines changed: 1 addition & 1 deletion
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