When available use AnnotationRegistry::registerUniqueLoader #25425

jrjohnson · 2017-12-10T22:02:16Z

Q	A
Branch?	3.3
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
References tickets	#24596
License	MIT

Take advantage of AnnotationRegistry::registerUniqueLoader which will only add 'class_exists' as an autoloader if it has not already been added. This helps alleviate a performance issue when the
same loader is added many times in tests.

nicolas-grekas

Once comment is addressed.

nicolas-grekas · 2017-12-10T22:05:01Z

src/Symfony/Bridge/PhpUnit/Legacy/SymfonyTestsListenerTrait.php

@@ -120,7 +120,11 @@ public function startTestSuite($suite)
            $this->state = 0;

            if (!class_exists('Doctrine\Common\Annotations\AnnotationRegistry', false) && class_exists('Doctrine\Common\Annotations\AnnotationRegistry')) {
-                AnnotationRegistry::registerLoader('class_exists');
+                if (method_exists(AnnotationRegistry::class, 'registerUniqueLoader')) {


The bridge is still PHP 5.3 compatible so this should be replaced by a regular string instead of:: class

But: does this actually do something? The first class_exists check just above already ensures unicity, isn't it?

Thanks, I made the string change.

The class now has two methods. In doctrine/annotations v1.6+ registerUniqueLoader exists and should be used, but it requires PHP 7.1+. This should support both depending on the consumers version of this library.

nicolas-grekas · 2017-12-10T22:11:25Z

There is another call to registerLoader in an XML file in FrameworkBundle, should be replaced also I believe when possible.

jrjohnson · 2017-12-10T22:12:11Z

I split into two PRs since this one could maintain BC and the change in the XML could not (as far as I can tell).

stof · 2017-12-11T09:27:35Z

Changing the registration in the XML file means you should alter the Definition object from the DI extension instead of registering it from the XML, to be able to choose the one being registered

stof · 2017-12-11T09:30:07Z

src/Symfony/Bridge/PhpUnit/Legacy/SymfonyTestsListenerTrait.php

@@ -269,7 +273,7 @@ public function endTest($test, $time)
            putenv('SYMFONY_DEPRECATIONS_SERIALIZE');
            foreach ($deprecations ? unserialize($deprecations) : array() as $deprecation) {
                if ($deprecation[0]) {
-                    trigger_error(serialize(array('deprecation' => $deprecation[1], 'class' => $className, 'method' => $test->getName(false))), E_USER_DEPRECATED);
+                    @trigger_error(serialize(array('deprecation' => $deprecation[1], 'class' => $className, 'method' => $test->getName(false))), E_USER_DEPRECATED);


this looks wrong to me, as it means the if and else blocks are now exactly the same.

that was suggested by fabbot, but should be reverted indeed, that's a false-positive

XML file needs update (modify the Definition object from the DI extension instead of registering this call from the XML file.)

jrjohnson · 2017-12-11T17:27:30Z

Thanks @stof and @nicolas-grekas. I reverted the false positive code style fix and moved the invocation on AnnotationRegistry into the configuration PHP. I'm not sure that is the right place for it so please let me know if I went the wrong direction.

nicolas-grekas · 2017-12-11T17:32:36Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/annotations.xml

-                <argument type="service">
-                    <!-- dummy arg to register class_exists as annotation loader only when required -->
-                    <service class="Doctrine\Common\Annotations\AnnotationRegistry">
-                        <call method="registerLoader">


you should keep this definition, and just replace "registerLoader" by "registerUniqueLoader" here.

THEN
in FrameworkExtension.php you need to fetch the "annotations.reader" definition using $container->getDefinition('annotations.reader'), and tweak it as needed to replace the call by "registerLoader" when the previous method doesn't exist.

Something like:

$container->getDefinition('annotations.reader') ->getMethodCalls()[0][1][1] ->setMethodCalls(array('registerLoader', array('class_exists')));

Ah ok, thanks. I would never have thought of that. I will make that change now.

Sorry @nicolas-grekas this is beyond my skill and understanding. $container->getDefinition('annotations.reader') ->getMethodCalls()[0][1][1] returns a Symfony\Component\DependencyInjection\Reference which doesn't have a setMethodCalls method. That method does exist on the Definition but I'm not sure how to duplicate the tiered structure in the XML or the call to a service.

Instead of an inlined service, you may move the service on L15 as a named one. That may help you, and future readers.

Looked back at the original code and doing this as a configurator was the suggested approach. Since I couldn't figure out how to do this another way I did it with a configurator. This has the added benefit of calling at static methods statically and not relying oh PHPs poor error control around calling them as instance methods.
I wasn't sure where to put the class though. I didn't want to create a special namespace just for it, so I shoehorned it into CacheWarmer which is probably doesn't belong.

That's reasonable, but actually we made it this way on purpose: to not add yet another class just for this.
I just pushed the change that makes this work as suggested :)

This method will only add 'class_exists' as an autoloader if it has not already been added. This helps alleviate a performance issue when the same loader is added many times in tests.

jrjohnson · 2017-12-12T23:49:27Z

Thanks for the assist @nicolas-grekas! A rebase seems to have cleared up the test issues so I believe this is ready.

fabpot · 2017-12-13T05:39:29Z

Thank you @jrjohnson.

…r (jrjohnson) This PR was merged into the 3.3 branch. Discussion ---------- When available use AnnotationRegistry::registerUniqueLoader | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | References tickets | #24596 | License | MIT Take advantage of AnnotationRegistry::registerUniqueLoader which will only add 'class_exists' as an autoloader if it has not already been added. This helps alleviate a performance issue when the same loader is added many times in tests. Commits ------- 97c3f42 Take advantage of AnnotationRegistry::registerUniqueLoader

carsonbot added Status: Needs Review Bug Status: Reviewed and removed Status: Needs Review labels Dec 10, 2017

nicolas-grekas previously approved these changes Dec 10, 2017

View reviewed changes

jrjohnson mentioned this pull request Dec 10, 2017

Use AnnotationRegistry::registerUniqueLoader in FrameworkBundle #25426

Closed

stof reviewed Dec 11, 2017

View reviewed changes

chalasr added this to the 3.3 milestone Dec 11, 2017

nicolas-grekas reviewed Dec 11, 2017

View reviewed changes

Take advantage of AnnotationRegistry::registerUniqueLoader

97c3f42

This method will only add 'class_exists' as an autoloader if it has not already been added. This helps alleviate a performance issue when the same loader is added many times in tests.

nicolas-grekas approved these changes Dec 13, 2017

View reviewed changes

fabpot approved these changes Dec 13, 2017

View reviewed changes

fabpot merged commit 97c3f42 into symfony:3.3 Dec 13, 2017

jrjohnson deleted the 24596-autoload-annotation branch December 13, 2017 05:41

This was referenced Dec 15, 2017

Release v3.4.2 #25505

Merged

Release v4.0.2 #25507

Merged

fabpot mentioned this pull request Jan 5, 2018

Release v3.3.15 #25690

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When available use AnnotationRegistry::registerUniqueLoader #25425

When available use AnnotationRegistry::registerUniqueLoader #25425

jrjohnson commented Dec 10, 2017

nicolas-grekas left a comment

nicolas-grekas Dec 10, 2017

nicolas-grekas Dec 10, 2017

jrjohnson Dec 10, 2017 •

edited

Loading

nicolas-grekas commented Dec 10, 2017

jrjohnson commented Dec 10, 2017

stof commented Dec 11, 2017

stof Dec 11, 2017

nicolas-grekas Dec 11, 2017

jrjohnson commented Dec 11, 2017

nicolas-grekas Dec 11, 2017 •

edited

Loading

nicolas-grekas Dec 11, 2017 •

edited

Loading

jrjohnson Dec 11, 2017

jrjohnson Dec 11, 2017

nicolas-grekas Dec 11, 2017

jrjohnson Dec 12, 2017

nicolas-grekas Dec 12, 2017 •

edited

Loading

jrjohnson commented Dec 12, 2017

fabpot commented Dec 13, 2017

When available use AnnotationRegistry::registerUniqueLoader #25425

When available use AnnotationRegistry::registerUniqueLoader #25425

Conversation

jrjohnson commented Dec 10, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrjohnson Dec 10, 2017 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Dec 10, 2017

jrjohnson commented Dec 10, 2017

stof commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrjohnson commented Dec 11, 2017

nicolas-grekas Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Dec 11, 2017 • edited Loading

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

nicolas-grekas Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

jrjohnson commented Dec 12, 2017

fabpot commented Dec 13, 2017

jrjohnson Dec 10, 2017 •

edited

Loading

nicolas-grekas Dec 11, 2017 •

edited

Loading

nicolas-grekas Dec 11, 2017 •

edited

Loading

nicolas-grekas Dec 12, 2017 •

edited

Loading