Skip to content

Commit c68ef46

Browse files
stofstof
committed
minor #30966 [DoctrineBridge] Deprecated using IdReader when optimization is not possible (HeahDude)
This PR was merged into the 4.3-dev branch. Discussion ---------- [DoctrineBridge] Deprecated using IdReader when optimization is not possible | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | yes <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | ~ | License | MIT | Doc PR | ~ Follow up of #30962. (Review only the second commit until #30962 is merged). Commits ------- a234c89 [DoctrineBridge] Deprecated using IdReader when optimization is not possible
2 parents c274dff + a234c89 commit c68ef46

File tree

6 files changed

+58
-54
lines changed

6 files changed

+58
-54
lines changed

‎UPGRADE-4.3.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,12 @@ DependencyInjection
3838
env(NAME): '1.5'
3939
```
4040
41+
Doctrine Bridge
42+
---------------
43+
44+
* Passing an `IdReader` to the `DoctrineChoiceLoader` when the query cannot be optimized with single id field has been deprecated, pass `null` instead
45+
* Not passing an `IdReader` to the `DoctrineChoiceLoader` when the query can be optimized with single id field has been deprecated
46+
4147
EventDispatcher
4248
---------------
4349

‎UPGRADE-5.0.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,9 @@ DoctrineBridge
6060

6161
* Deprecated injecting `ClassMetadataFactory` in `DoctrineExtractor`, an instance of `EntityManagerInterface` should be
6262
injected instead
63+
* Passing an `IdReader` to the `DoctrineChoiceLoader` when the query cannot be optimized with single id field will throw an exception, pass `null` instead
64+
* Not passing an `IdReader` to the `DoctrineChoiceLoader` when the query can be optimized with single id field will throw an exception
65+
6366

6467
DomCrawler
6568
----------

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ CHANGELOG
66

77
* changed guessing of DECIMAL to set the `input` option of `NumberType` to string
88
* deprecated not passing an `IdReader` to the `DoctrineChoiceLoader` when query can be optimized with a single id field
9+
* deprecated passing an `IdReader` to the `DoctrineChoiceLoader` when entities have a composite id
910

1011
4.2.0
1112
-----

‎src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,18 +42,25 @@ class DoctrineChoiceLoader implements ChoiceLoaderInterface
4242
*
4343
* @param ObjectManager $manager The object manager
4444
* @param string $class The class name of the loaded objects
45-
* @param IdReader $idReader The reader for the object IDs
45+
* @param IdReader|null $idReader The reader for the object IDs
4646
* @param EntityLoaderInterface|null $objectLoader The objects loader
4747
*/
4848
public function __construct(ObjectManager $manager, string $class, IdReader $idReader = null, EntityLoaderInterface $objectLoader = null)
4949
{
5050
$classMetadata = $manager->getClassMetadata($class);
5151

52+
if ($idReader && !$idReader->isSingleId()) {
53+
@trigger_error(sprintf('Passing an instance of "%s" to "%s" with an entity class "%s" that has a composite id is deprecated since Symfony 4.3 and will throw an exception in 5.0.', IdReader::class, __CLASS__, $class), E_USER_DEPRECATED);
54+
55+
// In Symfony 5.0
56+
// throw new \InvalidArgumentException(sprintf('The second argument `$idReader` of "%s" must be null when the query cannot be optimized because of composite id fields.', __METHOD__));
57+
}
58+
5259
if ((5 > \func_num_args() || false !== func_get_arg(4)) && null === $idReader) {
5360
$idReader = new IdReader($manager, $classMetadata);
5461

5562
if ($idReader->isSingleId()) {
56-
@trigger_error(sprintf('Not explicitly passing an instance of "%s" when it can optimize single id entity "%s" has been deprecated in 4.3 and will not apply any optimization in 5.0.', IdReader::class, $class), E_USER_DEPRECATED);
63+
@trigger_error(sprintf('Not explicitly passing an instance of "%s" to "%s" when it can optimize single id entity "%s" has been deprecated in 4.3 and will not apply any optimization in 5.0.', IdReader::class, __CLASS__, $class), E_USER_DEPRECATED);
5764
} else {
5865
$idReader = null;
5966
}
@@ -93,7 +100,7 @@ public function loadValuesForChoices(array $choices, $value = null)
93100

94101
// Optimize performance for single-field identifiers. We already
95102
// know that the IDs are used as values
96-
$optimize = null === $value || \is_array($value) && $value[0] === $this->idReader;
103+
$optimize = $this->idReader && (null === $value || \is_array($value) && $value[0] === $this->idReader);
97104

98105
// Attention: This optimization does not check choices for existence
99106
if ($optimize && !$this->choiceList && $this->idReader->isSingleId()) {

‎src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -163,13 +163,10 @@ public function configureOptions(OptionsResolver $resolver)
163163
};
164164

165165
$choiceName = function (Options $options) {
166-
/** @var IdReader $idReader */
167-
$idReader = $options['id_reader'];
168-
169166
// If the object has a single-column, numeric ID, use that ID as
170167
// field name. We can only use numeric IDs as names, as we cannot
171168
// guarantee that a non-numeric ID contains a valid form name
172-
if ($idReader->isIntId()) {
169+
if ($options['id_reader'] instanceof IdReader && $options['id_reader']->isIntId()) {
173170
return [__CLASS__, 'createChoiceName'];
174171
}
175172

@@ -181,12 +178,9 @@ public function configureOptions(OptionsResolver $resolver)
181178
// are indexed by an incrementing integer.
182179
// Use the ID/incrementing integer as choice value.
183180
$choiceValue = function (Options $options) {
184-
/** @var IdReader $idReader */
185-
$idReader = $options['id_reader'];
186-
187181
// If the entity has a single-column ID, use that ID as value
188-
if ($idReader->isSingleId()) {
189-
return [$idReader, 'getIdValue'];
182+
if ($options['id_reader'] instanceof IdReader && $options['id_reader']->isSingleId()) {
183+
return [$options['id_reader'], 'getIdValue'];
190184
}
191185

192186
// Otherwise, an incrementing integer is used as value automatically
@@ -240,7 +234,11 @@ public function configureOptions(OptionsResolver $resolver)
240234
$this->idReaders[$hash] = new IdReader($options['em'], $classMetadata);
241235
}
242236

243-
return $this->idReaders[$hash];
237+
if ($this->idReaders[$hash]->isSingleId()) {
238+
return $this->idReaders[$hash];
239+
}
240+
241+
return null;
244242
};
245243

246244
$resolver->setDefaults([

‎src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/DoctrineChoiceLoaderTest.php

Lines changed: 30 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,11 @@ protected function setUp()
8181
$this->idReader = $this->getMockBuilder('Symfony\Bridge\Doctrine\Form\ChoiceList\IdReader')
8282
->disableOriginalConstructor()
8383
->getMock();
84+
$this->idReader->expects($this->any())
85+
->method('isSingleId')
86+
->willReturn(true)
87+
;
88+
8489
$this->objectLoader = $this->getMockBuilder('Symfony\Bridge\Doctrine\Form\ChoiceList\EntityLoaderInterface')->getMock();
8590
$this->obj1 = (object) ['name' => 'A'];
8691
$this->obj2 = (object) ['name' => 'B'];
@@ -151,7 +156,7 @@ public function testLoadValuesForChoices()
151156
$loader = new DoctrineChoiceLoader(
152157
$this->om,
153158
$this->class,
154-
$this->idReader
159+
null
155160
);
156161

157162
$choices = [$this->obj1, $this->obj2, $this->obj3];
@@ -189,10 +194,6 @@ public function testLoadValuesForChoicesDoesNotLoadIfSingleIntId()
189194
$this->idReader
190195
);
191196

192-
$this->idReader->expects($this->any())
193-
->method('isSingleId')
194-
->willReturn(true);
195-
196197
$this->repository->expects($this->never())
197198
->method('findAll');
198199

@@ -215,10 +216,6 @@ public function testLoadValuesForChoicesLoadsIfSingleIntIdAndValueGiven()
215216
$choices = [$this->obj1, $this->obj2, $this->obj3];
216217
$value = function (\stdClass $object) { return $object->name; };
217218

218-
$this->idReader->expects($this->any())
219-
->method('isSingleId')
220-
->willReturn(true);
221-
222219
$this->repository->expects($this->once())
223220
->method('findAll')
224221
->willReturn($choices);
@@ -239,10 +236,6 @@ public function testLoadValuesForChoicesDoesNotLoadIfValueIsIdReader()
239236

240237
$value = [$this->idReader, 'getIdValue'];
241238

242-
$this->idReader->expects($this->any())
243-
->method('isSingleId')
244-
->willReturn(true);
245-
246239
$this->repository->expects($this->never())
247240
->method('findAll');
248241

@@ -303,10 +296,6 @@ public function testLoadChoicesForValuesLoadsOnlyChoicesIfSingleIntId()
303296

304297
$choices = [$this->obj2, $this->obj3];
305298

306-
$this->idReader->expects($this->any())
307-
->method('isSingleId')
308-
->willReturn(true);
309-
310299
$this->idReader->expects($this->any())
311300
->method('getIdField')
312301
->willReturn('idField');
@@ -343,10 +332,6 @@ public function testLoadChoicesForValuesLoadsAllIfSingleIntIdAndValueGiven()
343332
$choices = [$this->obj1, $this->obj2, $this->obj3];
344333
$value = function (\stdClass $object) { return $object->name; };
345334

346-
$this->idReader->expects($this->any())
347-
->method('isSingleId')
348-
->willReturn(true);
349-
350335
$this->repository->expects($this->once())
351336
->method('findAll')
352337
->willReturn($choices);
@@ -369,10 +354,6 @@ public function testLoadChoicesForValuesLoadsOnlyChoicesIfValueIsIdReader()
369354
$choices = [$this->obj2, $this->obj3];
370355
$value = [$this->idReader, 'getIdValue'];
371356

372-
$this->idReader->expects($this->any())
373-
->method('isSingleId')
374-
->willReturn(true);
375-
376357
$this->idReader->expects($this->any())
377358
->method('getIdField')
378359
->willReturn('idField');
@@ -398,7 +379,7 @@ public function testLoadChoicesForValuesLoadsOnlyChoicesIfValueIsIdReader()
398379
/**
399380
* @group legacy
400381
*
401-
* @expectedDeprecation Not explicitly passing an instance of "Symfony\Bridge\Doctrine\Form\ChoiceList\IdReader" when it can optimize single id entity "%s" has been deprecated in 4.3 and will not apply any optimization in 5.0.
382+
* @expectedDeprecation Not explicitly passing an instance of "Symfony\Bridge\Doctrine\Form\ChoiceList\IdReader" to "Symfony\Bridge\Doctrine\Form\ChoiceList\DoctrineChoiceLoader" when it can optimize single id entity "%s" has been deprecated in 4.3 and will not apply any optimization in 5.0.
402383
*/
403384
public function testLoaderWithoutIdReaderCanBeOptimized()
404385
{
@@ -445,14 +426,6 @@ public function testLoaderWithoutIdReaderCanBeOptimized()
445426

446427
$choices = [$obj1, $obj2];
447428

448-
$this->idReader->expects($this->any())
449-
->method('isSingleId')
450-
->willReturn(true);
451-
452-
$this->idReader->expects($this->any())
453-
->method('getIdField')
454-
->willReturn('idField');
455-
456429
$this->repository->expects($this->never())
457430
->method('findAll');
458431

@@ -461,13 +434,29 @@ public function testLoaderWithoutIdReaderCanBeOptimized()
461434
->with('idField', ['1'])
462435
->willReturn($choices);
463436

464-
$this->idReader->expects($this->any())
465-
->method('getIdValue')
466-
->willReturnMap([
467-
[$obj1, '1'],
468-
[$obj2, '2'],
469-
]);
470-
471437
$this->assertSame([$obj1], $loader->loadChoicesForValues(['1']));
472438
}
439+
440+
/**
441+
* @group legacy
442+
*
443+
* @deprecationMessage Passing an instance of "Symfony\Bridge\Doctrine\Form\ChoiceList\IdReader" to "Symfony\Bridge\Doctrine\Form\ChoiceList\DoctrineChoiceLoader" with an entity class "stdClass" that has a composite id is deprecated since Symfony 4.3 and will throw an exception in 5.0.
444+
*/
445+
public function testPassingIdReaderWithoutSingleIdEntity()
446+
{
447+
$idReader = $this->createMock(IdReader::class);
448+
$idReader->expects($this->once())
449+
->method('isSingleId')
450+
->willReturn(false)
451+
;
452+
453+
$loader = new DoctrineChoiceLoader(
454+
$this->om,
455+
$this->class,
456+
$idReader,
457+
$this->objectLoader
458+
);
459+
460+
$this->assertInstanceOf(DoctrineChoiceLoader::class, $loader);
461+
}
473462
}

0 commit comments

Comments
 (0)