Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[DoctrineBridge] Add support for doctrine/persistence 4#59575
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Let me know if this is the wrong branch, I'm never quite sure for this kind of case with Symfony, but in the past, I think I've been asked to contribute this kind of branch as a patch. |
| if (method_exists(ObjectManager::class, 'isUninitializedObject')) { | ||
| $this->markTestSkipped('This test is not applicable to doctrine/persistence 4'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How does Persistence 4 behave in this situation? Should we have a test for that as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@derrabusTypeError in the signature of theAbstractManagerRegistry when passingnull as default manager name. AndInvalidArgumentException if there is no manager with that name.
@greg0ire we cannot remove support for the case of having no entity manager. As the ManagerRegistry interface extends ConnectionRegistry in doctrine/persistence (which is a mistake to me, but won't be an easy fix) and we don't have any standalone implementation of ConnectionRegistry, DoctrineBundle will still create a manager registry even when your project enables DBAL and not ORM. And so this case exists (it won't be a case of having no manager registry at all)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the proper fix is to fix the mock of the ManagerRegistry. When passingnull as argument here,getManagerForClass should indeed returnnull butgetManager should throw anInvalidArgumentException (which is already the behavior of doctrine/persistence 2.x and 3.x btw)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the pointers, I'll try that 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This PR is broken in its current state. It should update mocks to match the actual behavior of doctrine/persistence instead of skipping relevant tests.
-1 for merging it in its current state.
| @@ -50,6 +50,7 @@ public function resolve(Request $request, ArgumentMetadata $argument): array | |||
| if (!$options->class || $options->disabled) { | |||
| return []; | |||
| } | |||
| // doctrine/persistence < 4 compat | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is not true.$this->getManager can still returnnull (see the implementation of the private method). This method call isnot a call to the doctrine/persistence method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh right, didn't spot that!
| if (method_exists(ObjectManager::class, 'isUninitializedObject')) { | ||
| $this->markTestSkipped('This test is not applicable to doctrine/persistence 4'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@derrabusTypeError in the signature of theAbstractManagerRegistry when passingnull as default manager name. AndInvalidArgumentException if there is no manager with that name.
@greg0ire we cannot remove support for the case of having no entity manager. As the ManagerRegistry interface extends ConnectionRegistry in doctrine/persistence (which is a mistake to me, but won't be an easy fix) and we don't have any standalone implementation of ConnectionRegistry, DoctrineBundle will still create a manager registry even when your project enables DBAL and not ORM. And so this case exists (it won't be a case of having no manager registry at all)
| if (method_exists(ObjectManager::class, 'isUninitializedObject')) { | ||
| $this->markTestSkipped('This test is not applicable to doctrine/persistence 4'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the proper fix is to fix the mock of the ManagerRegistry. When passingnull as argument here,getManagerForClass should indeed returnnull butgetManager should throw anInvalidArgumentException (which is already the behavior of doctrine/persistence 2.x and 3.x btw)
53877b8 tob09aa33Compare| $registry->expects($this->any()) | ||
| ->method('getManager') | ||
| ->willReturn($manager); | ||
| if (method_exists(ObjectManager::class, 'isUninitializedObject')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should check$manager === null. We don't want to throw when we have an instance.
And I suggest applying this condition even for older versions of doctrine/persistence so that the mock reflects the actual behavior of the class. AbstractManagerRegistry was already throwing an exception in persistence 2.x (I did not check the behavior in 1.x)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I did not check the behavior in 1.x
It is not allowed by Symfony'scomposer.json 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I know. That's exactly why I did not check 1.x to see whether our mocks were already wrong back in time or whether they kept emulating the 1.x behavior, because distinguishing those 2 cases does not matter.
| @@ -635,6 +639,10 @@ public function testDedicatedEntityManagerNullObject() | |||
| public function testEntityManagerNullObject() | |||
| { | |||
| if (method_exists(ObjectManager::class, 'isUninitializedObject')) { | |||
| $this->markTestSkipped('This test is not applicable to doctrine/persistence 4'); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Those tests are application, for the same reason that the EntityValueResolver test (and with the same issue about a bad definition of the mock)
| @@ -71,6 +71,7 @@ public function validate(mixed $entity, Constraint $constraint) | |||
| if ($constraint->em) { | |||
| $em = $this->registry->getManager($constraint->em); | |||
| // doctrine/persistence < 4 compat | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this comment is actually wrong. Older versions ofdoctrine/persistence are also throwing an exception ingetManager
d0069f8 to0b58efaComparesrc/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
v4 provides a guarantee that ManagerRegistry::getManager() returns an entitymanager (as opposed to null).The tests need to be adjusted to reflect the behavior of the mocked dependencymore accurately, as it is throwing an exception in case of a null manager forall three supported versions of the library.
0b58efa to6c20172CompareThank you@greg0ire. |
ef15506 intosymfony:6.4Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
v4 provides a guarantee that
ManagerRegistry::getManager()returns an entity manager (as opposed to null).