Skip to content

[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

Closed
wants to merge 1 commit into from

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Feb 10, 2022

Q A
Branch? 6.1
Bug fix? no
New feature? no
Deprecations? no
Tickets Fix #44364
License MIT
Doc PR -

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:

  • leave 4.4 to 6.0 branches as is, thus allowing ppl to benefit from preloading at their own risk
  • bump 6.1 to php 8.1 and free ourselves and our users from the issue for the future.

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

@derrabus
Copy link
Member

How would you feel about merging this change to 6.1 instead of 4.4?

@nicolas-grekas
Copy link
Member Author

I hesitated. I think I'd be fine with it. Calling @jderusse for his opinion about that.

@nicolas-grekas nicolas-grekas changed the title [DependencyInjection] Skip preloading on PHP 8.0 [DependencyInjection] Skip preloading on PHP 7.4/8.0 Feb 10, 2022
@nicolas-grekas
Copy link
Member Author

(PR updated to account for PHP 7.4, which has the same issue)

@jderusse
Copy link
Member

Is it possible to trigger a deprecation or reporting this in the error_log?

How would you feel about merging this change to 6.1 instead of 4.4?

Actually, this PR fixes a bug in symfony/di: 4.4 => the generated preload script could trigger errors in PHP when class has typed props. It could be a symfony 6.1 package or in whatever packages.

I'm not sure why we would only fix it in symfony/di 6.1.

@Tobion
Copy link
Contributor

Tobion commented Feb 10, 2022

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.
So this would solve

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.

@nicolas-grekas nicolas-grekas changed the base branch from 4.4 to 6.1 February 10, 2022 18:12
@nicolas-grekas nicolas-grekas changed the title [DependencyInjection] Skip preloading on PHP 7.4/8.0 [DependencyInjection] Skip preloading on PHP 8.0 Feb 10, 2022
@nicolas-grekas nicolas-grekas modified the milestones: 4.4, 6.1 Feb 10, 2022
@nicolas-grekas
Copy link
Member Author

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.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Feb 11, 2022

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

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.

@jderusse
Copy link
Member

see scheb/2fa#121 for real-world reference

@nicolas-grekas
Copy link
Member Author

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.

@nicolas-grekas
Copy link
Member Author

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

@nicolas-grekas nicolas-grekas deleted the nopreloadonphp8 branch February 25, 2022 10:27
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.

Typehinted properties on missing class with preloading
5 participants