Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When available use AnnotationRegistry::registerUniqueLoader #25425

Merged
merged 1 commit into from Dec 13, 2017

Conversation

@jrjohnson
Copy link
Contributor

jrjohnson commented Dec 10, 2017

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.

Copy link
Member

nicolas-grekas left a comment

Once comment is addressed.

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 10, 2017

Member

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 10, 2017

Member

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

This comment has been minimized.

Copy link
@jrjohnson

jrjohnson Dec 10, 2017

Author Contributor

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

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 10, 2017

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

@jrjohnson

This comment has been minimized.

Copy link
Contributor Author

jrjohnson commented Dec 10, 2017

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).

@jrjohnson jrjohnson force-pushed the jrjohnson:24596-autoload-annotation branch from 8641f71 to e84d120 Dec 10, 2017
@stof

This comment has been minimized.

Copy link
Member

stof commented Dec 11, 2017

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

@@ -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 comment has been minimized.

Copy link
@stof

stof Dec 11, 2017

Member

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 11, 2017

Member

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

@nicolas-grekas nicolas-grekas dismissed their stale review Dec 11, 2017

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

@jrjohnson jrjohnson force-pushed the jrjohnson:24596-autoload-annotation branch from 1a2a93f to acc4f0d Dec 11, 2017
@jrjohnson

This comment has been minimized.

Copy link
Contributor Author

jrjohnson commented Dec 11, 2017

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.

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 11, 2017

Member

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.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 11, 2017

Member

Something like:

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

This comment has been minimized.

Copy link
@jrjohnson

jrjohnson Dec 11, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@jrjohnson

jrjohnson Dec 11, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 11, 2017

Member

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

This comment has been minimized.

Copy link
@jrjohnson

jrjohnson Dec 12, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 12, 2017

Member

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 :)

@jrjohnson jrjohnson force-pushed the jrjohnson:24596-autoload-annotation branch 3 times, most recently from 78a6cbc to ceabd2c Dec 12, 2017
@nicolas-grekas nicolas-grekas force-pushed the jrjohnson:24596-autoload-annotation branch from ceabd2c to 7ba9ac4 Dec 12, 2017
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 jrjohnson force-pushed the jrjohnson:24596-autoload-annotation branch from 7ba9ac4 to 97c3f42 Dec 12, 2017
@jrjohnson

This comment has been minimized.

Copy link
Contributor Author

jrjohnson commented Dec 12, 2017

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

@fabpot
fabpot approved these changes Dec 13, 2017
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Dec 13, 2017

Thank you @jrjohnson.

@fabpot fabpot merged commit 97c3f42 into symfony:3.3 Dec 13, 2017
2 of 3 checks passed
2 of 3 checks passed
fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabpot added a commit that referenced this pull request Dec 13, 2017
…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
@jrjohnson jrjohnson deleted the jrjohnson:24596-autoload-annotation branch Dec 13, 2017
This was referenced Dec 15, 2017
@fabpot fabpot mentioned this pull request Jan 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.