RFC: Drop PHP 7.3-support in all future Flow versions

I’m with Alex on this one and would suggest to go for the “conservative” solution:
The typed class properties for example have a risk of breaking existing things (no offense ofc, but it is a rather low-level change with a lot of touchpoints). I’d like to use that feature, but for LTS versions we should probably focus on stabililty.

I would consider maintaining a separate version 7.4 noch that much effort other than the initial hassle to create the branches. It maily is one extra branch to consider for upmerges and an extra release job to trigger.

I’m a bit unhappy with the poll… the first thing should be a one on one decision: Should the requirements stay frozen or not. Because now two of the options for the “no” votings of this questions get distributed, painting a distorted image of the intentions.

I would agree to anything, but the first question. I hope we take this into account instead of “just” following the winning answer!

1 Like

Aaaand: I cannot take in part in the vote. I guess my User is missing the Role “TeamMember”

I added you to the “Member” group

Its still saying:

You need to be a member of NeosTeam to vote in this poll.

I agree to @fcool that the result is biased by basically splitting the no options into two.

I know quite some Hosters and projects that may run into trouble in a minor update because of that. I also know some good examples of hosters that proactively contact customers and urge them to update but not all customers use those.

I strongly advocate against raising the php version in a bugfix release.
As i undestand it, this defeates the purpose of LTS completely and will cause a lot of grief. What happens if there is a security patch for Neos 7.3? This would be a problem without updating PHP right? In that case this is a no go.
Please dont do it :pray:

1 Like

Thank you for your comments – even though, we probably should have discussed this before starting to vote.

Don’t be afraid of the poll result, there’s no law that we need to take the result as it is, we are still allowed to do what makes sense.

Still, I wanted to clarify one thought:

If we decide to continue supporting PHP 7.3, I’d target my PR for class property type support for Flow 8.0 instead of Flow 7.3 (unless someone volunteers to backport it in a clean way for PHP 7.3). That would leave us with an incomplete support for PHP 7.4 in Flow 7.3.x. Not really dramatic, we didn’t have that support previously.

However, for the packages I maintain, I don’t want to invest the extra work to support different shades of PHP 7.4 syntaxes. My mindset is already focused on PHP 8.1, so it takes a bit of effort to remember 7.4 times. But Phpstorm helps me to check if my code is also PHP 7.4 compatible. What Phpstorm cannot do though is to check if my code works with the specific PHP 7.4 syntax which is supported by Flow 7.3.

/**
  * @Flow\Inject
  * @var Dependency
  */
protected ?Dependency $dependency = null;

:point_up: this won’t work with Flow 7.3

So I just imagine what could go wrong when I upmerge changes and something goes wrong with type hints.

Conclusion: I’d rather not invest the time to support Flow < 8.0 for the packages I maintain.

I’d support Flow 7.4, if there is one which requires PHP 7.4. But I don’t see the big advantage, why someone would upgrade from Flow 7.3 to 7.4 and not to 8.0 right away.

I’m just concerned about the time invest. I can’t imagine that there are more than a handful of users, if any, using a security-patched PHP 7.3 with Flow 7.3. I don’t know any company doing that, not even our enterprise customers do that. And if you use PHP 7.3 without custom security patches, then what’s the big deal to Flow 7.3 without patches? It’s YOLO anyway :wink:

Anyway, I think we got everything on the table now. I guess question is mostly if there enough people who’d like to invest energy into supporting a Flow 7.4 or not. What do you think?

I still think that is all subjective conjecture, we cannot know the numbers, and we don’t know how long distributions might provide security patches to PHP versions in their distributions, making that scenario a potential “safe” scenario to have. I still think Flow 7.4 with the PHP 7.4 support, is the best compromise for everything and the work amount shouldn’t be that big, upmerges between flow 7.3 and 7.4 should be a non issue and hitting one more release button as well.

That all said, I also don#t know the numbers but also suspect it will be a small number that benefits from this. I guess focussing on Flow 8 and having 3rd party packages moving support to Flow >=8 is an option I would be fine with which also pushes updates.

Are you sure about that?
I just tested this exact syntax and it works for me with Flow 7.3 (and PHP 7.4)!

What does not work is if you omit the @var annotation, but even the = null initialization can be removed.


Personally I’d have no problem to drop support for PHP < 7.4 in an LTS version, since that version is officially dead thus people really can’t argue with security.

But I would have issues with a substantial change to the proxy class building in a patch level release.

Maybe we can have a minimal-invasive version that just supports dropping the @var annotation in favor of a type hint though.

Are you sure about that?
I just tested this exact syntax and it works for me with Flow 7.3 (and PHP 7.4)!

Lazy dependencies will break. Check the full example in the description of the PR: FEATURE: Lazy injections for typed properties by robertlemke · Pull Request #2701 · neos/flow-development-collection · GitHub

Anyway, since there was no review activity yet, I think you are right that it’s to complex for a patch level release. On the other hand, I spent more than four days working on it (even if it doesn’t look like that), and I’d like to use it. Therefore I’ll start to rebase this for Flow 8.0, so we may get it ready in time for the feature freeze.

4 Likes

You’re right. The example above works only if Dependency is a prototype. Otherwise I’ll have to add a lazy=false to the Flow\Inject annotation (or use constructor injection ofc).

I’m sorry if you feel frustrated. I really value your efforts (and I think we all do) and your PR seems like a big improvement in general!

Awesome, thanks!

2 Likes

Thanks! I’m also aware, that I should be more humble with this contribution. I can’t make this one PR a big deal (and I try not to) while the rest of the team is busy with many other important changes. I know that I’m currently a low-frequency-code-contributor and appreciate how passionate and diligent you folks work on Neos and Flow. Thanks a lot for that!

4 Likes

Coming back to this after about 9 months, realizing we never actually did anything… and by now PHP 7.4 joined the club of dead PHP versions.

So, what do we do? Do we need to do something?

I would be fine with leaving flow 7 as it is but improve the php 8 support and use on the 8.x branch that never promised to support php 7.

Personally think that we should be a little more aggressive with adding strict types to flow where we can.

3 Likes

Yes :partying_face:

We did raise minimum PHP version to 8 in Flow 8, no? So I think we did do something :tada:

Regarding the “^8.0” and this somehow implicitly demanding us to add newer 8.x PHP versions to the build pipeline, I think we should consider to avoid doing “^” version matches, at least for PHP. But that’s a bit of a different topic that would need to be discussed separately. There are valid reasons for and against.

Personally think that we should be a little more aggressive with adding strict types to flow where we can.

Absolutely. The “where we can” is the hard part though :slight_smile: There’s lots of hidden breakiness involved (see my efforts back in time), but I’m all for being agressive in the next major release if someone is up to the task of doing this (hint: psalm has some support for adding this automatically, but ofc needs some vetting)

Yes, but given this was about dropping 7.3… :man_shrugging: But yeah.

Maybe we should amend our versioning policy to say we stop supporting PHP versions when they reach EOL. :thinking:

Hm. I oppose and say we should have added PHP 8.2 to our pipleine before it got released to make the needed adjustments. I usually frown upon not testing beta/RC releases, so we should make use of them (in the future). The release process for PHP 8.2 was not a surprise.

Then again, once a version constraint has been opened, it’s too late to dial back, as composer would simply fall back to the newest version that still claims support. So I’d agree to

  • for PHP 9 only do "php": "~9.0.0" and “open up” later combined with
  • testing new minor PHP versions as soon as available in GH Actions
1 Like

I think that is the better strategy. Opening up our semver policy to allow for arbitrary breaking changes to update the syntax is not benefitting our users, but only our own need/wish to stay up to date (and maintainable), while we don’t have the capacity to pull it off cleanly within one major release cycle (which with our current branching strategy is ~3months, so yes, let’s fix that!).

say we should have added PHP 8.2 to our pipleine before it got released

That’s not in opposition :slight_smile: I agree! And that’s part of what I intended with the “nightly” build runs that should be able to fail without blocking merges (TASK: Enable PHP nightly build by albe · Pull Request #2524 · neos/flow-development-collection · GitHub)

  • for PHP 9 only do "php": "~9.0.0" and “open up” later combined with
  • testing new minor PHP versions as soon as available in GH Actions

:+1: