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

[FEATURE] Re-establish support for PHP 7.2 #1208

Closed
wants to merge 1 commit into from
Closed

Conversation

oliverklee
Copy link
Contributor

This reverts commit 9cd1e60.

Fixes #1207

@oliverklee oliverklee added this to the 8.0.0 milestone Jan 16, 2023
@oliverklee oliverklee self-assigned this Jan 16, 2023
@oliverklee oliverklee marked this pull request as draft January 16, 2023 16:15
@oliverklee
Copy link
Contributor Author

We need to downgrade to PHPUnit 8.x first, it seems.

@JakeQZ
Copy link
Contributor

JakeQZ commented Jan 17, 2023

There may have been some breaking changes between PHPUnit 8.x and 9.x. IIRC, some of the assert methods were renamed, with the old names deprecated. It may be that we do need to drop support for PHP 7.2, if it's not possible for our test code to be compatible with all the versions of PHPUnit that would be needed to support such a wide range range of PHP versions.

The principle was not to drop support for older PHP versions if there's no reason to do so. But if there is a reason, so be it.

@mvorisek
Copy link

Yes, please do not drop PHP support too early. This library prevented us to upgrade PHP for Prestashop which uses v5.x which is limited to PHP 8.0 for no big reason.

@JakeQZ
Copy link
Contributor

JakeQZ commented Jul 28, 2023

This library prevented us to upgrade PHP for Prestashop which uses v5.x which is limited to PHP 8.0 for no big reason.

It took me a few minutes to work out your point. I have now got it.

I also disagree with setting a maximum PHP version number for our software. If we didn't, we wouldn't have to constantly update it for every new PHP release.

@oliverklee's argument is that otherwise we would be releasing software that has not been tested against a future PHP version, but nontheless claims to work with it.

Prestashop could up the version of Emogrifier to 6.0. Or use Composer to select and install a suitable version. I think the issue you have is with Prestashop.

@oliverklee
Copy link
Contributor Author

oliverklee commented Jul 28, 2023

We also could provide maintenance versions (with bug fixes and support for higher PHP versions) of e.g. the previous major release. However, someone would need to put the time into that to maintain these branches (mostly backport PRs, and create PRs for adding support and CI for higher PHP versions). @JakeQZ @mvorisek Would you two be willing to do this?

@JakeQZ
Copy link
Contributor

JakeQZ commented Sep 7, 2023

@JakeQZ @mvorisek Would you two be willing to do this?

(I replied the GitHub notification email with a comment, which I thought would get posted here. I guess that feature of GitHub doesn't work, or maybe it wasn't intended to work like that.)

Basically, no, (I don't have that extra time.)

But I think addressing #1210 should solve most of these types of issues for the future, in an automated way, so we wouldn't have to do anything more than that (moving forwards).

@mvorisek, thanks for sharing your experience; useful feedback and good to know. Feel free to further comment on #1210 there.

Re this PR, I think due to the breaking changes between various major versions of PHPUnit (and maybe other compatibility issues), re-supporting PHP 7.2 would be too much effort to be viable given that the already-small benefit will fade over time.

So I'm happy to close this (as willnotimplement or suchlike). @oliverklee, thanks for giving it a try.

@oliverklee
Copy link
Contributor Author

Okay, agreed. Closing.

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.

Re-establish support for PHP 7.2
3 participants