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

Allow installation on PHP 8.1 #62

Merged
merged 3 commits into from
Nov 26, 2021
Merged

Allow installation on PHP 8.1 #62

merged 3 commits into from
Nov 26, 2021

Conversation

cedric-anne
Copy link
Contributor

Q A
Documentation no
Bugfix no
BC Break no
New Feature yes
RFC no
QA no

Description

Allow installation on PHP 8.1.

As discussed on #61 , a temporary polyfill was used in dev context to be able to test against PHP 8.1.

@froschdesign froschdesign self-assigned this Oct 26, 2021
@froschdesign froschdesign added Enhancement hacktoberfest-accepted Issues/Pull-Requests which can be fixed during Hacktoberfest: https://hacktoberfest.digitalocean.com labels Oct 26, 2021
@froschdesign froschdesign added this to the 2.12.0 milestone Oct 26, 2021
@Ocramius
Copy link
Member

@boesing could you check this one, when possible? Unsure about the polyfill implications :D

@froschdesign
Copy link
Member

@Ocramius
The related fix is already present in laminas-cache-storage-adapter-memory but not yet released:

https://github.com/laminas/laminas-cache-storage-adapter-memory/blob/2.0.x/src/Memory.php#L117-L122

Some more work on laminas-cache itself and the adapters is needed to provide full support of PHP 8.1.
This means we can wait or use the same solution with polyfills as in laminas-view.

Another option is to remove the caching feature from the translator and use PSR 16 via a decorator – which is already planned for the next major version. But that means some more work and time.

@cedric-anne
Copy link
Contributor Author

Ping @boesing

@boesing
Copy link
Member

boesing commented Nov 3, 2021

Even tho this could work, upstream projects which rely on laminas-cache might be stuck on PHP 8.0 due to the fact that PHP 8.1 support in laminas-cache v2 is something I've just considered after realizing that there are so many projects which rely on laminas-cache and its StorageFactory quirk.

Since v3 of laminas-cache does not have that static factory magic anymore, I definitely have to think about an MVC module which polyfills the StorageFactory. Maybe another v2.14.0 release with a PHP 8.1 backport (which will also include several minor releases in laminas-cache-storage-adapter* components) is something I have to consider as well as I don't think that the upgrade path to v3 is doable without having breaks in all other components which do work with laminas-cache StorageFactory static factory.


Regarding the polyfill, if it makes tests green and its only a dev autoloader quirk, I don't really care. But this won't change anything regarding the laminas-cache topic.

@froschdesign
Copy link
Member

Regarding the polyfill, if it makes tests green and its only a dev autoloader quirk, I don't really care. But this won't change anything regarding the laminas-cache topic.

It is definitely only for the completion of the tests and we must inform that the cache support is not ready for 8.1.

@cedric-anne
Copy link
Contributor Author

laminas-cache is just a suggested dependency. As PHP 8.1 is not allowed by its composer.json file, people will be informed of this lack of support when they will try to install/require it with composer on a PHP 8.1 platform.
Also, I guess on PHP 8.1 only deprecation warnings are triggered. These warning may be a pain in CI as they may result in tests failing when strictiness is pushed to its maximum, but except maybe on some specific adapters, cache should work as expected on PHP 8.1.

@froschdesign
Copy link
Member

I think we can simplify the update and skip the unit tests with cache for PHP 8.1.

@boesing What do you think?

@boesing
Copy link
Member

boesing commented Nov 3, 2021

@froschdesign would be fine with both 👌🏻

@cedric-anne
Copy link
Contributor Author

Hi,

I removed the laminas-cache memory adapter polyfill and disable related tests on PHP >= 8.1.0-dev.

Also, I added a commit to fix deprecated usage of null arguments in IntlDateFormatter::__construct(). Indeed, even if PHP Doc indicates these parameters could be set to null, PHP code says something else.

Is there something else I should/can do ?

Regards

@froschdesign
Copy link
Member

@cedric-anne
I think that I will be able to create a new version at the weekend, which will contain some more fixes.

Thanks for your help! ❤️

@cedric-anne
Copy link
Contributor Author

Also, I added a commit to fix deprecated usage of null arguments in IntlDateFormatter::__construct(). Indeed, even if PHP Doc indicates these parameters could be set to null, PHP code says something else.

For this one, I created a bug on PHP bugtracker, and I proposed a PR on PHP source.
I do not really know if it would be accepted, but in this case, my second commit will be useless.

@pascalheidmann
Copy link

Any update on this PR? Would love to see it fixed so I can fix mezzio/mezzio-laminasrouter#9 which builds on laminas-i18n 8.1 support ;)

Copy link
Member

@froschdesign froschdesign left a comment

Choose a reason for hiding this comment

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

The new package laminas-cache-storage-deprecated-factory can be used to fix the problem with the deprecated storage factory.

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

 > - Downloading squizlabs/php_codesniffer (2.9.2)

This is very old, and likely the reason why you are getting failures on CS checks 🤔

@froschdesign
Copy link
Member

@Ocramius

This is very old, and likely the reason why you are getting failures on CS checks 🤔

The reason:

"laminas/laminas-coding-standard": "~1.0.0",

@cedric-anne
Copy link
Contributor Author

@Ocramius

This is very old, and likely the reason why you are getting failures on CS checks thinking

The reason:

"laminas/laminas-coding-standard": "~1.0.0",

I'm working on it. Problem appears when dropping PHP 7.3 support. Maybe it should be done in another PR, as there are many CS fixes to do to be able to upgrade laminas/laminas-coding-standard.

@froschdesign
Copy link
Member

@cedric-anne

Maybe it should be done in another PR, as there are many CS fixes to do to be able to upgrade laminas/laminas-coding-standard.

If it helps here then no problem! 👍

@cedric-anne
Copy link
Contributor Author

cedric-anne commented Nov 24, 2021

It is OK now. I upgraded all dev dependencies to versions compatible with PHP ^7.3 || ~8.0.0 or ^7.3 || ~8.0.0 || ~8.1.0 and used the --prefer-lowest flag to ensure that all of them are retrieved in their PHP 7.3 compatible version.

PS: sorry for the force-push mess.

@froschdesign froschdesign linked an issue Nov 26, 2021 that may be closed by this pull request
@froschdesign froschdesign merged commit 9fe2242 into laminas:2.12.x Nov 26, 2021
@froschdesign
Copy link
Member

@cedric-anne
Thank you for your time and this contribution! 👍

@cedric-anne cedric-anne deleted the feature/php-8.1 branch November 26, 2021 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement hacktoberfest-accepted Issues/Pull-Requests which can be fixed during Hacktoberfest: https://hacktoberfest.digitalocean.com
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Works not with laminas-cache >= 3.0.0
5 participants