Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Lazy configuration of annotations' loader and@required#21837
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
311abd7 to977b1b3Compare| <serviceid="annotations.reader"class="Doctrine\Common\Annotations\AnnotationReader"public="false"> | ||
| <argument>null</argument> | ||
| <argumenttype="service"> | ||
| <!-- dummy arg to register class_exists as annotation loader only when required--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
such dummy arg could cause issue if the library adds an optional arg in the future.
Thus, I think PHP used to do weird things regarding the evaluation of useless constructor arguments.
We should use a configurator instead of using such hack.
977b1b3 toc1ce50aComparenicolas-grekas commentedMar 2, 2017
@stof I moved the argument to the |
fabpot commentedMar 2, 2017
👍 Does the job |
dunglas commentedMar 3, 2017
👍 |
c1ce50a tod332b37Compare| </argument> | ||
| </call> | ||
| </service> | ||
| <serviceid="Doctrine\Common\Annotations\Reader"alias="annotations.reader"public="false" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
shouldn't this targetannotation_reader?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it's overridden in FrameworkExtension (but please investigate if you'd like, unrelated to this PR of course)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, I just noticed it while reviewing your PR. I'll check it.
GuilhemN left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
fabpot commentedMar 9, 2017
The clean way would be to use a configurator, which means adding one additional class. Would it be better? Do we just merge it like this? In any case, I like the idea of removing the need for an I'm inclined to just merge this PR as is, but some @symfony/deciders might think going the clean way would be preferable. What's your thoughts? |
fabpot commentedMar 15, 2017
Thank you@nicolas-grekas. |
…oader and `@required` (nicolas-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[FrameworkBundle] Lazy configuration of annotations' loader and `@required`| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -This would remove the need forsymfony/symfony-standard#1052 and for the `autoload.php` file altogether.Tested on symfony-demo with great success so far.Commits-------d332b37 [FrameworkBundle] Lazy configuration of annotations' loader and `@required`
backbone87 commentedApr 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Its basically a hack? How about some general container-driven scripted config: <container ... > <services> <serviceid="annotations.reader"class="Doctrine\Common\Annotations\AnnotationReader"public="false"> ... </service> <services> <scripttype="php/closure"depends="annotations.reader"on-missing="skip|force|error"run-on="build|load"><![CDATA[ Doctrine\Common\Annotations\AnnotationRegistry::registerLoader('class_exists');]]></script></container> Edit: I will create a new ticket for it |
… is absent (BPScott)This PR was merged into the 5.0.x-dev branch.Discussion----------Allow building bootstrap.php.cache when app/autoload.php is absentIf app/autoload.php is absent then use the autoload.php in Composer'svendor directory.symfony/symfony#21837 opens the door for removing `app/autoload.php` in Symfony apps. I tried removing that and updating the various reference to it in symfony-standard and my app works (:tada:), but when I run composer install/update it complains about the buildBootstrap task expecting to find `app/autoload.php`.This fix tells the buildBootstrap task to use `vendor/autoload.php` if `app/autoload.php` is absent.Commits-------257ee2e Allow building bootstrap.php.cache when app/autoload.php is absent
This PR was merged into the 3.3-dev branch.Discussion----------Remove app/autoload.phpNow thatsymfony/symfony#21837 is merged, app/autoload.php can be removed and replaced with the standard Composer autoloader.~WAIT! This can't be merged *just* yet, as it throws errors when running the `buildBootstrap` post install/update script. The fix for this is insensiolabs/SensioDistributionBundle#313. That PR must be merged and this PR must be updated to use a version of SensioDistributionBundle that contains the fix before this is good to go.~EDIT:sensiolabs/SensioDistributionBundle#313 is merged (as of 24/04), and this PR has been updated to use the latest version of SensioDistributionBundle. This is now good to mergeCommits-------298f8b2 Remove app/autoload.php
This would remove the need forsymfony/symfony-standard#1052 and for the
autoload.phpfile altogether.Tested on symfony-demo with great success so far.