[FrameworkBundle] Lazy configuration of annotations' loader and @required
#21837
Conversation
311abd7 to
977b1b3
Compare
| <service id="annotations.reader" class="Doctrine\Common\Annotations\AnnotationReader" public="false"> | ||
| <argument>null</argument> | ||
| <argument type="service"> | ||
| <!-- dummy arg to register class_exists as annotation loader only when required --> |
There was a problem hiding this comment.
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 to
c1ce50a
Compare
|
@stof I moved the argument to the |
|
👍 Does the job |
|
👍 |
c1ce50a to
d332b37
Compare
| </service> | ||
| </argument> | ||
| </call> | ||
| </service> | ||
| <service id="Doctrine\Common\Annotations\Reader" alias="annotations.reader" public="false" /> |
There was a problem hiding this comment.
shouldn't this target annotation_reader?
There was a problem hiding this comment.
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.
Yes, I just noticed it while reviewing your PR. I'll check it.
|
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? |
|
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 for symfony/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`
|
Its basically a hack? How about some general container-driven scripted config: <container ... >
<services>
<service id="annotations.reader" class="Doctrine\Common\Annotations\AnnotationReader" public="false">
...
</service>
<services>
<script type="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 absent If app/autoload.php is absent then use the autoload.php in Composer's vendor 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.php Now that symfony/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 in sensiolabs/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 merge Commits ------- 298f8b2 Remove app/autoload.php
This would remove the need for symfony/symfony-standard#1052 and for the
autoload.phpfile altogether.Tested on symfony-demo with great success so far.