Refactoring EntityUserProvider::__construct() to not do work, cause cache warm error #16772

weaverryan · 2015-12-01T05:28:19Z

Q	A
Bug fix?	"yes"
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	none
License	MIT
Doc PR	n/a

This fixes a "Database not Found" error when using doctrine/orm 2.5 while warming up your cache under certain situations. Basically, if you use the EntityUserProvider, then during cache warmup, Twig requires the security.authorization_checker which eventually requires this EntityUserProvider, which previously caused Doctrine to calculate the metadata for your User class. If no database exists (and you haven't specified the platform), you'll get the error.

More broadly, this simply tries to do less work in the constructor. It's a "bug fix", only kind of, but as it's completely an internal refactoring, it should be quite safe.

Thanks!

stof · 2015-12-01T09:53:09Z

instead of loadMetadata, I suggest getting the repository and the metadata from the entity manager only on demand (getting it several times is not an issue as the entity manager keeps them in memory and so the second access will just read them from the array). This would simplify lots of things (and initializing $this->class requires loading the metadata only when it contains a : to resolve the alias)

stof · 2015-12-01T09:53:58Z

src/Symfony/Bridge/Doctrine/Security/User/EntityUserProvider.php

-        }
-
-        $this->repository = $em->getRepository($class);
+        $this->em = $registry->getManager($managerName);


even better: you should store the registry and the manager name instead of instantiating the entity manager here. This would also make it compatible with resetting the EM.

weaverryan · 2015-12-01T16:21:48Z

src/Symfony/Bridge/Doctrine/Tests/Security/User/EntityUserProviderTest.php

@@ -125,7 +125,7 @@ public function testLoadUserByUserNameShouldDeclineInvalidInterface()
    private function getManager($em, $name = null)
    {
        $manager = $this->getMock('Doctrine\Common\Persistence\ManagerRegistry');
-        $manager->expects($this->once())
+        $manager->expects($this->any())


We shouldn't really care how many times getManager() is called anyways. And now, it's called multiple times in some tests, and zero times in others.

weaverryan · 2015-12-01T16:23:28Z

@stof good suggestions - I've just updated this PR: we get everything lazily. This is still an internal refactoring - I tried to minimize the changes to make this obvious.

Now, would this be a bug fix for 2.3? I have the PR opened against 2.8... which doesn't make any sense :). It seems like this should go into 2.3 or into master for 3.1. I'd like to put it into 2.3.

Tobion · 2015-12-01T18:03:22Z

Does this fix doctrine/DoctrineBundle#351 ?

Tobion · 2015-12-01T18:08:56Z

src/Symfony/Bridge/Doctrine/Security/User/EntityUserProvider.php

+
+    private function getClassMetadata()
+    {
+        return $this->getEntityManager()->getClassMetadata($this->getClass());


getClassMetadata calls getClass and getClass calls getClassMetadata. Looks like a possible endless loop.

Wow, good eye. In fact, I should only use getClass() where we actually need a valid PHP class name. I've updated to do that (otherwise, just use the class/alias that was passed in).

weaverryan · 2015-12-01T21:58:14Z

@Tobion Yes and no. It definitely fixed one more case where that error happens. But are there other situations that still cause that error? Possibly. But yea, when I saw this issue locally, I knew it was doctrine/DoctrineBundle#351 and wanted to squash it :)

nicolas-grekas · 2015-12-01T22:06:07Z

👍 for 2.3

Tobion · 2015-12-02T00:19:06Z

The only other place where getClassMetadata or getRepository is called in a constructor in symfony is for the ChoiceLoader in Doctrine form type. But that is only constructed on demand when evaluating choices. So that should not be a problem. So i think after this change symfony itself is save against doctrine/DoctrineBundle#351

👍 for 2.3

Status: Reviewed

xabbuh · 2015-12-02T10:17:41Z

👍

stof · 2015-12-02T11:53:28Z

src/Symfony/Bridge/Doctrine/Security/User/EntityUserProvider.php

+        return $class === $this->getClass() || is_subclass_of($class, $this->getClass());
+    }
+
+    private function getEntityManager()


I would call it getObjectManager, as this also supports ODMs

stof · 2015-12-02T11:55:42Z

I agree this should go in 2.3

…ny work This helps break a dependency chain during cache warmup, that includes having Doctrine load metadata, which will fail if there is no database on new Doctrine versions.

…e issue

stof · 2015-12-02T15:18:26Z

👍

weaverryan · 2015-12-02T15:18:30Z

I've just rebased this against 2.3 - it shows as a conflict against 2.8, but will merge cleanly into 2.3.

It should be ready for a merge. Tests before my last rebase all passed except for an unrelated hhvm failure.

stof · 2015-12-02T15:36:02Z

Thank you @weaverryan.

…rk, cause cache warm error (weaverryan) This PR was submitted for the 2.8 branch but it was merged into the 2.3 branch instead (closes #16772). Discussion ---------- Refactoring EntityUserProvider::__construct() to not do work, cause cache warm error | Q | A | ------------- | --- | Bug fix? | "yes" | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | none | License | MIT | Doc PR | n/a This fixes a "Database not Found" error when using `doctrine/orm` 2.5 while warming up your cache under certain situations. Basically, if you use the `EntityUserProvider`, then during cache warmup, Twig requires the `security.authorization_checker` which eventually requires this `EntityUserProvider`, which previously caused Doctrine to calculate the metadata for your User class. If no database exists (and you haven't specified the platform), you'll get the error. More broadly, this simply tries to do less work in the constructor. It's a "bug fix", only kind of, but as it's completely an internal refactoring, it should be quite safe. Thanks! Commits ------- 44a2861 Refactoring EntityUserProvider::__construct() to not do work, cause cache warm error

weaverryan · 2015-12-02T15:53:45Z

Thanks @stof! There will be a conflict when merging to 2.8: but it should be simple to fix: use the new side of the diff (i.e. the code from 2.8), but replace $this->repository with $repository (and don't forget the $repository that's way over to the right at the end of a message :)

stof · 2015-12-02T16:07:06Z

I merge 2.3 into 2.7 and 2.8 to resolve the conflict

carsonbot added the Status: Needs Review label Dec 1, 2015

stof reviewed Dec 1, 2015
View reviewed changes

weaverryan force-pushed the lazy-action-entity-user-provider branch 2 times, most recently from 43a8c73 to 4d6d41d Compare December 1, 2015 16:20

weaverryan reviewed Dec 1, 2015
View reviewed changes

Tobion reviewed Dec 1, 2015
View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 2, 2015

stof reviewed Dec 2, 2015
View reviewed changes

weaverryan added 2 commits December 2, 2015 10:15

Refactoring EntityUserProvider::__construct() so that it doesn't do a…

8e01383

…ny work This helps break a dependency chain during cache warmup, that includes having Doctrine load metadata, which will fail if there is no database on new Doctrine versions.

Only using getClass() where I actually need a class. Fixes a recursiv…

b4246bf

…e issue

weaverryan force-pushed the lazy-action-entity-user-provider branch from ca8b843 to b4246bf Compare December 2, 2015 15:16

stof closed this Dec 2, 2015

weaverryan deleted the lazy-action-entity-user-provider branch December 14, 2015 22:50

This was referenced Dec 26, 2015

Release v2.3.36 #17141

Merged

Release v2.7.8 #17142

Merged

This was referenced Dec 26, 2015

Release v2.8.1 #17143

Merged

Release v3.0.1 #17144

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring EntityUserProvider::__construct() to not do work, cause cache warm error #16772

Refactoring EntityUserProvider::__construct() to not do work, cause cache warm error #16772

weaverryan commented Dec 1, 2015

stof commented Dec 1, 2015

stof Dec 1, 2015

weaverryan Dec 1, 2015

weaverryan commented Dec 1, 2015

Tobion commented Dec 1, 2015

Tobion Dec 1, 2015

weaverryan Dec 1, 2015

weaverryan commented Dec 1, 2015

nicolas-grekas commented Dec 1, 2015

Tobion commented Dec 2, 2015

xabbuh commented Dec 2, 2015

stof Dec 2, 2015

weaverryan Dec 2, 2015

stof commented Dec 2, 2015

stof commented Dec 2, 2015

weaverryan commented Dec 2, 2015

stof commented Dec 2, 2015

weaverryan commented Dec 2, 2015

stof commented Dec 2, 2015

Refactoring EntityUserProvider::__construct() to not do work, cause cache warm error #16772

Refactoring EntityUserProvider::__construct() to not do work, cause cache warm error #16772

Conversation

weaverryan commented Dec 1, 2015

stof commented Dec 1, 2015

stof Dec 1, 2015

Choose a reason for hiding this comment

weaverryan Dec 1, 2015

Choose a reason for hiding this comment

weaverryan commented Dec 1, 2015

Tobion commented Dec 1, 2015

Tobion Dec 1, 2015

Choose a reason for hiding this comment

weaverryan Dec 1, 2015

Choose a reason for hiding this comment

weaverryan commented Dec 1, 2015

nicolas-grekas commented Dec 1, 2015

Tobion commented Dec 2, 2015

xabbuh commented Dec 2, 2015

stof Dec 2, 2015

Choose a reason for hiding this comment

weaverryan Dec 2, 2015

Choose a reason for hiding this comment

stof commented Dec 2, 2015

stof commented Dec 2, 2015

weaverryan commented Dec 2, 2015

stof commented Dec 2, 2015

weaverryan commented Dec 2, 2015

stof commented Dec 2, 2015