Skip to content
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

Make php-8.1 minimum requirement #4124

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Jul 28, 2024

We removed 7.3 support in 03/2003 ... now - plus one year later - its time to make the next step.

PHP8 offers a lot of good features, i'd like to use when writing extension but i am limited due to php7 support.

Worth a read ... https://accesto.com/blog/php-performance-improvement-features/

According to packagist.org stats only 20% still use php7.4 with latest releases.

https://packagist.org/packages/openmage/magento-lts/php-stats#20.10


Note 1: If you have modules/extensions that dont work with php8, drop me a note! This should be no reason to stay on an old php version!

Note 2: OpenMage will still proberly work with older versions too - as long we not introduce backwards-incomaptible changes.

Note3 : with https://github.com/rectorphp/rector you have a tool to refactor your code for php8.

sreichel added 5 commits July 22, 2024 14:01
* Rector: CQ - UnusedForeachValueToArrayKeysRector

See Rector\CodeQuality\Rector\Foreach_\UnusedForeachValueToArrayKeysRector

* fixes + phpstan

See fix at rector: rectorphp/rector-src#6164
- updated workflows
- phpstan baseline (will be fixed later)
@sreichel sreichel changed the title Make php-8.,0 minimum requirement Make php-8.0 minimum requirement Jul 28, 2024
@sreichel sreichel marked this pull request as draft July 28, 2024 14:58
sreichel and others added 3 commits August 7, 2024 06:09
Co-authored-by: Ng Kiat Siong <kiatsiong.ng@gmail.com>
# Conflicts:
#	composer.lock
#	phpstan.dist.baseline.neon
@github-actions github-actions bot added Component: lib/Varien Relates to lib/Varien Component: Media Relates to Mage_Media Component: Install Relates to Mage_Install Component: lib/* Relates to lib/* labels Sep 4, 2024
@sreichel sreichel changed the title Make php-8.0 minimum requirement Make php-8.1 minimum requirement Sep 4, 2024
@sreichel sreichel marked this pull request as ready for review September 4, 2024 23:20
addison74
addison74 previously approved these changes Sep 5, 2024
@sreichel sreichel marked this pull request as draft September 5, 2024 23:05
@colinmollenhour
Copy link
Member

colinmollenhour commented Sep 9, 2024

Agreed with the changes, but I believe this should be in "next" so that it is in the next major version update.

MAJOR version when you make incompatible API changes

  • ...
  • dependencies updated in major ways (e.g. dropping support of PHP 7.4 or MySQL 5.6)

All new PRs are based on either "main" branch for MINOR and PATCH updates or "next" branch for MAJOR updates

Since v21 was not released yet this could still make it into v21.

EDIT: Just saw there is already #3920

@sreichel
Copy link
Contributor Author

sreichel commented Sep 9, 2024

@colinmollenhour its on draft, b/c we should think about to raise minimum version for main branch too.

# Conflicts:
#	.github/workflows/phpstan.yml
#	.github/workflows/sonar.yml
#	.github/workflows/syntax-php.yml
#	app/code/core/Mage/Media/Model/File/Image.php
#	composer.json
#	composer.lock
#	phpstan.dist.baseline.neon
@sreichel
Copy link
Contributor Author

Moving to php8.1 is no BC-breaking change. You only cant update 20,13 (?).

v21/next contains breaking changes ... we cant use it as it is.

Avoid maintaining 2 versions concurrently

20, 21, 22? Why not continue with v20 to keep it simple?

@kiatng
Copy link
Contributor

kiatng commented Feb 17, 2025

20, 21, 22? Why not continue with v20 to keep it simple?

PHP8 has breaking changes and deprecations, so to me, it makes sense to have it in v21. It will reflect the major updates and ensure clarity for users and developers about the compatibility requirements.

@addison74
Copy link
Contributor

addison74 commented Feb 17, 2025

As a personal opinion, I would like to refer to OpenMage as one only version, with the possibility of releasing new versions periodicaly. I would use the project's front page or a pin to the discussions to prepare the community for major changes in advance, so that all those interested in upgrading can read about and take actions. For example, on date X the minimum PHP version will be 8.1, on date Y Prototype library will be removed from OpenMage. This v21/next would act more like a testing version until the new PRs are safe to be used in the main branch. All the changes that could generate BCs will be discussed in public then a deadline is set. Maybe there are people who want to sponsor some useful implementations and then those who implement them can be paid for their work.

I have been using PHP 8.3 and 8.4 for some time now, both in production and testing. Regarding this PR, I would add it first to the next branch (for testing), setting a date for a new version of OpenMage (for example July 1st 2025), get ready a warning effective from today and attach a discussion section for it. If there are no differences of opinion, on the set date PHP 8.1 becomes the minimum PHP for OpenMage.

@sreichel
Copy link
Contributor Author

20, 21, 22? Why not continue with v20 to keep it simple?

PHP8 has breaking changes and deprecations, so to me, it makes sense to have it in v21. It will reflect the major updates and ensure clarity for users and developers about the compatibility requirements.

What do you think about that?

https://medium.com/@sampart/semantic-versioning-when-you-change-the-required-programming-language-version-16a3a3555c95

Conclusion
When you change the version of PHP supported by a project using Semantic Versioning, increment your minor version number.

@sreichel
Copy link
Contributor Author

there must be a v21 to remove PHP <8.1 since that would break ~42% of installations according to packagist.

Please check where the 42% came from. Its from users that run older OM-releases. Please see numbers for v20.11/20.12.

And ... a lot of current PRs are about fixing php8 errors/deprecations. Not much other fixes. If you need php74, you only/mostly miss some updates for php8.

@kiatng
Copy link
Contributor

kiatng commented Feb 19, 2025

Ref the medium article on PHP and release versioning. How about our own house rules:
image

@sreichel
Copy link
Contributor Author

How about our own house rules:

In all that years i cant remember to have agreed to any of that RFCs! Imho .... thats something for larger projects.

@sreichel
Copy link
Contributor Author

Closed due to concerns.

@sreichel sreichel closed this Feb 19, 2025
@colinmollenhour
Copy link
Member

@sven, the Medium article you referenced makes a good case for it to be MINOR, one that I didn't think of before.. The fact that the composer package can prevent a user with the wrong PHP from upgrading is interesting - although in some cases I have used --ignore-platform-reqs so this would get me in trouble..

The purpose of the RFC is to make it clear to users what version specification it is safe for them to use given their updating practices.

Given that we have ceased to support non-composer installations (we no longer build a zip file), I could see taking this approach if everyone agrees to allow PHP requirements to change in a MINOR version so long as they are specified correctly in the composer.json file.

This would go against the Release Schedule guide, but if everyone wants it that way and there are no strong arguments against it, I would support this change to the release schedule document so it becomes official.

i cant remember to have agreed to any of that RFCs

There were 11 votes Yea and 0 Nea after months of discussion, so not sure what to say to that... OpenMage/rfcs#10

Those are the rules currently, but it doesn't mean the rules can't change, just gotta make a proposal and get everyone onboard.

@sreichel
Copy link
Contributor Author

i cant remember to have agreed to any of that RFCs

There were 11 votes Yea and 0 Nea after months of discussion, so not sure what to say to that... OpenMage/rfcs#10

I only said that i not agreed/participated. For me it is too much for a small project like this.

@sreichel sreichel removed the MAJOR This PR is intended to be treated as a MAJOR update according to RFC-0002 label Feb 20, 2025
@sreichel
Copy link
Contributor Author

just gotta make a proposal and get everyone onboard.

My proposal was this PR. We have 72/27% out of 18 votes.

For me it does not matter how the branch is called. We can bump the major version, but we cant use v21 b/c of breaking-changes ...

@colinmollenhour
Copy link
Member

Looks like OpenMage is broken on PHP 8.0 so it seems already in practice the minimum version is 8.1... 🤣

PHP Parse error:  syntax error, unexpected token ")" in .../vendor/symfony/css-selector/XPath/Extension/NodeExtension.php on line 63

Given we have a currently "broken" build and the strong arguments for allowing PHP engine requirements to be a MINOR version change instead of MAJOR, I propose amending the Release Schedule and merging this PR. I'll follow up with a PR for the release schedule.

Copy link
Member

@colinmollenhour colinmollenhour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving since it is already not compatible with PHP 8.0 due to some previous update.

@colinmollenhour
Copy link
Member

Added PR to amend the release schedule document: OpenMage/rfcs#12

@sreichel
Copy link
Contributor Author

looks like OpenMage is broken on PHP 8.0 so it seems already in practice the minimum version is 8.1... 🤣

This should not happen, Can you confirm your symfony/css-selector is on v5.4.45?

@kiatng
Copy link
Contributor

kiatng commented Feb 23, 2025

I have one of the production servers on PHP7.4 running OM20.10.2. The composer lock entry:

            "require": {
                "ext-dom": "*",
                "ext-libxml": "*",
                "php": "~7.3.0 || ~7.4.0 || ~8.0.0 || ~8.1.0 || ~8.2.0 || ~8.3.0 || ~8.4.0",
                "sabberworm/php-css-parser": "^8.7.0",
                "symfony/css-selector": "^4.4.23 || ^5.4.0 || ^6.0.0 || ^7.0.0"
            },

I have not encountered any issue.

@sreichel
Copy link
Contributor Author

sreichel commented Feb 23, 2025

composer.lock should also have this

            "name": "symfony/css-selector",
            "version": "v5.4.45",
            "source": {
                "type": "git",
                "url": "https://github.com/symfony/css-selector.git",
                "reference": "4f7f3c35fba88146b56d0025d20ace3f3901f097"
            },

@colinmollenhour
Copy link
Member

Ahh, maybe it's my mistake, then... I was trying to update the demo server and ran composer update, but maybe I ran it locally on a later PHP version than is used by the Nexcess server. Daniel updated the Nexcess config to use PHP 8.3 since then, so it's working now, but sounds like it would've been fine on PHP 8.0 had I run the composer update through Github actions. Oops...

# Conflicts:
#	.github/workflows/phpunit.yml
#	.github/workflows/release.yml
#	composer.lock
@sreichel sreichel marked this pull request as draft March 5, 2025 23:06
@addison74
Copy link
Contributor

I would go a little further by setting the minimum PHP version to 8.2 in 3 -4 months from now. I have been using this version for a long time in a test environment and all the problems that arose I reported and they were resolved.

@colinmollenhour
Copy link
Member

Yes, at this point 8.1 is EOL in just 9 months.. although if we're only doing minor versions for PHP upgrades, then it won't be so hard to update again later.

Copy link

sonarqubecloud bot commented Mar 6, 2025

@colinmollenhour
Copy link
Member

There are currently several errors preventing the checks from passing...

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.

4 participants