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

Don't register the class_exists loader #23420

Closed
wants to merge 1 commit into from
Closed

Don't register the class_exists loader #23420

wants to merge 1 commit into from

Conversation

jrjohnson
Copy link
Contributor

@jrjohnson jrjohnson commented Jul 6, 2017

Q A
Branch? 3.3,0
Bug fix? yes
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes (but this seems to be untested code)
Fixed tickets #23419
License MIT

This is now done when the container boots in Symfony 3.3.x.

#21837 registers this loader now. Registering it twice is expensive as it gets called a LOT. Especially during tests. In a comparison of Symfony 3.2.10 and 3.3.4 a single test was calling class_exists 90K more times.

https://blackfire.io/profiles/compare/0c3627c5-8432-4908-b097-1289b81d63bb/graph

This isn't the best solution, but I'm not sure how to detect the version of Symfony from within the bootstrap.php file so this would be a BC break for any version of Symfony before 3.3.

I also created doctrine/annotations#135
to address this, but wanted to cover all bases.

This is now done when the container boots in Symfony 3.3.x.
@@ -20,10 +19,6 @@
// Enforce a consistent locale
setlocale(LC_ALL, 'C');

if (!class_exists('Doctrine\Common\Annotations\AnnotationRegistry', false) && class_exists('Doctrine\Common\Annotations\AnnotationRegistry')) {
AnnotationRegistry::registerLoader('class_exists');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To preserve BC, I think we should keep registering this, but add a check in the above if to not do it when 3.3 is in use (dunno which check exactly right now.)

@jrjohnson
Copy link
Contributor Author

I actually think maybe this doesn't matter too much, the fix in doctrine/annotations is the important bit. A lot of my functional tests boot kernels and all of them end up adding class_exists to the AnnotationRegistry so by the end of a test group there are 25+ of these registered. The single one in the PHPUnit bridge isn't really that important. If there is a way to detect the Symfony version then this is probable still worth doing, but it isn't as helpful as I first thought.

@nicolas-grekas
Copy link
Member

Ok, closing then, thanks for investigating.

@jrjohnson jrjohnson deleted the double-class-exists branch July 6, 2017 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants