-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DependencyInjection] Skip preloading on PHP 8.0 #45384
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
Conversation
How would you feel about merging this change to 6.1 instead of 4.4? |
I hesitated. I think I'd be fine with it. Calling @jderusse for his opinion about that. |
2190386
to
c798909
Compare
(PR updated to account for PHP 7.4, which has the same issue) |
Is it possible to trigger a deprecation or reporting this in the
Actually, this PR fixes a bug in I'm not sure why we would only fix it in symfony/di 6.1. |
I'm fine with bumping the php requirement as in #45377. It could be a good idea to do both: Increase php requirement in 6.1 to increase maintainability and skip preloading in older php versions so people are not affected by this common problem, e.g. if it happens in other libraries. But I would suggest to skip preloading in the recipe https://github.com/symfony/recipes/blob/master/symfony/framework-bundle/5.4/config/preload.php instead. This way, people can still use preloading in 7.4/8.0 if they are not affected by optional dependency types by just removing the condition.
|
c798909
to
6d378ec
Compare
6d378ec
to
2f2d4d0
Compare
2f2d4d0
to
845873f
Compare
I rebased this PR for 6.1: the more I think about it, the more I see merging this on 4.4 as something that will break the perf of perfectly fine infras, while fixing an issue that is yet to be reported by userland. I think the topic for branches < 6.1 should be dealt with separately, and should be based on real-world feedback. |
that's a nice idea, but here also I wouldn't take a preemptive approach. If the issue is super rare on < 6.1 the correct approach could very well be to tell to disable preloading on their own. Let's wait for the issue to be reported in real world before doing anything. |
see scheb/2fa#121 for real-world reference |
Thanks for the link @jderusse that's interesting. I still think we should not break the perf of existing infras lightly, but of course this topic remains open. |
Closing as I don't think we should break the perf of perfectly fine infras lightly and #45377 is going to solve this for 6.1+. |
This PR replaces #44380, which IMHO is great, but is complexity we'd better not add to the codebase.
Instead, @jderusse has proposed to disable preloading for PHP 8.0 from 4.4. This PR does it.
The drawback of this approach is that many apps that don't use typed properties would benefit from the perf boost provided by preloading on PHP 7.4/8.0. But I still prefer this over merging #44380.
Another approach I took in #45377 is to:
I prefer #45377 but others in the core-team prefer to follow the existing Symfony policy: "don't bump in a minor", which I appreciate too. Not easy :)