[DoctrineBridge] Add support for doctrine/persistence 4 #59575

greg0ire · 2025-01-21T21:43:42Z

v4 provides a guarantee that ManagerRegistry::getManager() returns an entity manager (as opposed to null).

Q	A
Branch?	6.4
Bug fix?	no
New feature?	no
Deprecations?	no
Issues	n/a
License	MIT

greg0ire · 2025-01-21T21:44:48Z

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.

derrabus · 2025-01-22T07:40:22Z

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

+        if (method_exists(ObjectManager::class, 'isUninitializedObject')) {
+            $this->markTestSkipped('This test is not applicable to doctrine/persistence 4');
+        }


How does Persistence 4 behave in this situation? Should we have a test for that as well?

@derrabus TypeError in the signature of the AbstractManagerRegistry when passing null as default manager name. And InvalidArgumentException 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)

the proper fix is to fix the mock of the ManagerRegistry. When passing null as argument here, getManagerForClass should indeed return null but getManager should throw an InvalidArgumentException (which is already the behavior of doctrine/persistence 2.x and 3.x btw)

Thanks for the pointers, I'll try that 👍

stof

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.

stof · 2025-01-22T14:35:11Z

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

@@ -50,6 +50,7 @@ public function resolve(Request $request, ArgumentMetadata $argument): array
        if (!$options->class || $options->disabled) {
            return [];
        }
+        // doctrine/persistence < 4 compat


This is not true. $this->getManager can still return null (see the implementation of the private method). This method call is not a call to the doctrine/persistence method.

Oh right, didn't spot that!

stof · 2025-01-22T14:42:03Z

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

+        if (method_exists(ObjectManager::class, 'isUninitializedObject')) {
+            $this->markTestSkipped('This test is not applicable to doctrine/persistence 4');
+        }


@derrabus TypeError in the signature of the AbstractManagerRegistry when passing null as default manager name. And InvalidArgumentException 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)

stof · 2025-01-22T14:44:46Z

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

+        if (method_exists(ObjectManager::class, 'isUninitializedObject')) {
+            $this->markTestSkipped('This test is not applicable to doctrine/persistence 4');
+        }


the proper fix is to fix the mock of the ManagerRegistry. When passing null as argument here, getManagerForClass should indeed return null but getManager should throw an InvalidArgumentException (which is already the behavior of doctrine/persistence 2.x and 3.x btw)

greg0ire · 2025-01-22T19:50:01Z

@stof I've addressed your review.

Also, I forgot to mention it, but doctrine/persistence 4 is now indeed installed in at least this job

stof · 2025-01-23T12:21:10Z

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

-        $registry->expects($this->any())
-            ->method('getManager')
-            ->willReturn($manager);
+        if (method_exists(ObjectManager::class, 'isUninitializedObject')) {


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)

I did not check the behavior in 1.x

It is not allowed by Symfony's composer.json 👍

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.

stof · 2025-01-23T12:22:51Z

src/Symfony/Bridge/Doctrine/Tests/Validator/Constraints/UniqueEntityValidatorTest.php

@@ -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');


Those tests are application, for the same reason that the EntityValueResolver test (and with the same issue about a bad definition of the mock)

stof · 2025-01-23T12:23:45Z

src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.php

@@ -71,6 +71,7 @@ public function validate(mixed $entity, Constraint $constraint)
        if ($constraint->em) {
            $em = $this->registry->getManager($constraint->em);

+            // doctrine/persistence < 4 compat


this comment is actually wrong. Older versions of doctrine/persistence are also throwing an exception in getManager

src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.php

v4 provides a guarantee that ManagerRegistry::getManager() returns an entity manager (as opposed to null). The tests need to be adjusted to reflect the behavior of the mocked dependency more accurately, as it is throwing an exception in case of a null manager for all three supported versions of the library.

fabpot · 2025-01-25T08:05:08Z

Thank you @greg0ire.

carsonbot added the Status: Needs Review label Jan 21, 2025

carsonbot added this to the 6.4 milestone Jan 21, 2025

GromNaN added the DoctrineBridge label Jan 21, 2025

carsonbot changed the title ~~Add support for doctrine/persistence 4~~ [DoctrineBridge] Add support for doctrine/persistence 4 Jan 21, 2025

greg0ire mentioned this pull request Jan 21, 2025

Make compatible with doctrine/persistence 4.0 doctrine/DoctrineBundle#1833

Open

GromNaN approved these changes Jan 22, 2025

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 22, 2025

derrabus reviewed Jan 22, 2025

View reviewed changes

stof requested changes Jan 22, 2025

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Jan 22, 2025

GromNaN self-requested a review January 22, 2025 15:01

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jan 22, 2025

greg0ire force-pushed the persistence-4-compat branch from 53877b8 to b09aa33 Compare January 22, 2025 18:03

greg0ire requested a review from stof January 22, 2025 18:15

stof reviewed Jan 23, 2025

View reviewed changes

greg0ire force-pushed the persistence-4-compat branch 3 times, most recently from d0069f8 to 0b58efa Compare January 23, 2025 14:29

greg0ire requested a review from stof January 23, 2025 14:49

greg0ire commented Jan 23, 2025

View reviewed changes

src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.php Show resolved Hide resolved

stof approved these changes Jan 23, 2025

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 23, 2025

fabpot approved these changes Jan 25, 2025

View reviewed changes

src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.php Outdated Show resolved Hide resolved

fabpot force-pushed the persistence-4-compat branch from 0b58efa to 6c20172 Compare January 25, 2025 08:05

fabpot merged commit ef15506 into symfony:6.4 Jan 25, 2025
6 of 7 checks passed

This was referenced Jan 29, 2025

Release v6.4.18 #59642

Merged

Release v7.1.11 #59643

Merged

Release v7.2.3 #59644

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DoctrineBridge] Add support for doctrine/persistence 4 #59575

[DoctrineBridge] Add support for doctrine/persistence 4 #59575

greg0ire commented Jan 21, 2025 •

edited by GromNaN

Loading

greg0ire commented Jan 21, 2025

derrabus Jan 22, 2025

stof Jan 22, 2025

stof Jan 22, 2025

greg0ire Jan 22, 2025

stof left a comment

stof Jan 22, 2025

greg0ire Jan 22, 2025

stof Jan 22, 2025

stof Jan 22, 2025

greg0ire commented Jan 22, 2025

stof Jan 23, 2025

greg0ire Jan 23, 2025

stof Jan 23, 2025

stof Jan 23, 2025

stof Jan 23, 2025

fabpot commented Jan 25, 2025

[DoctrineBridge] Add support for doctrine/persistence 4 #59575

[DoctrineBridge] Add support for doctrine/persistence 4 #59575

Conversation

greg0ire commented Jan 21, 2025 • edited by GromNaN Loading

greg0ire commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jan 25, 2025

greg0ire commented Jan 21, 2025 •

edited by GromNaN

Loading