search

laminas / laminas-server

Create Reflection-based RPC servers
https://docs.laminas.dev/laminas-server/
BSD 3-Clause "New" or "Revised" License
25 stars 13 forks source link

Provide PHP 8.1 support #30

Closed ihor-sviziev closed 2 years ago

ihor-sviziev commented 2 years ago
Q A
Documentation no
Bugfix no
BC Break no
New Feature no
RFC no
QA no

Description

Fixes https://github.com/magento/magento2/issues/34240

ihor-sviziev commented 2 years ago

Not sure how to trigger all tests

ihor-sviziev commented 2 years ago

@froschdesign could you explain how to trigger the test run?

froschdesign commented 2 years ago

@ihor-sviziev Is done automatically. I don't know why it doesn't work.

ihor-sviziev commented 2 years ago

ok. let's close and re-open it again, maybe it will work

froschdesign commented 2 years ago

@ihor-sviziev Works now!

froschdesign commented 2 years ago

@ihor-sviziev You can drop version 7.3 of PHP, there is no need to support outdated versions.

ihor-sviziev commented 2 years ago

@froschdesign looks like for draft PRs tests are not getting ran :(

froschdesign commented 2 years ago

@ihor-sviziev

looks like for draft PRs tests are not getting ran :(

No, this has nothing to do with the draft status. It's just a little delayed right now and there are some other problems here on GitHub.

ihor-sviziev commented 2 years ago

@froschdesign, it looks like laminas-code 4.4.3 still doesn't support PHP 8.1 (but tests are passing with it), but if we're ignoring platform requirements - it allows to install laminas-code 3.5.* that doesn't support even PHP 8.0 and tests are failing. Should we wait for release with PHP 8.1 in the laminas-code?

Another option - increase minimum required version of laminas-code to 4.x, but that will drop the PHP 7.3 support - I think that's not expected.

froschdesign commented 2 years ago

@ihor-sviziev

Another option - increase minimum required version of laminas-code to 4.x, but that will drop the PHP 7.3 support - I think that's not expected.

Please see https://github.com/laminas/laminas-server/pull/30#issuecomment-937645687

ihor-sviziev commented 2 years ago

ok... now we removed php 7.3 support and phpcs started running on PHP 7.4, and showing deprecations :(
Updated to "laminas/laminas-coding-standard": "^2.1.4", similar to laminas-code

Ocramius commented 2 years ago

Looks like the CS installer didn't really work?

 Error: Start tag expected, '<' not found on line 1, column 1

ERROR: Referenced sniff "./vendor/laminas/laminas-coding-standard/ruleset.xml" does not exist
ihor-sviziev commented 2 years ago

Ok... looks like we should port https://github.com/laminas/laminas-server/pull/7 to 2.10 now.

froschdesign commented 2 years ago

@ihor-sviziev

Ok... looks like we should port #7 to 2.10 now.

This pull request will be released with the next minor version: 2.11.0

ihor-sviziev commented 2 years ago

Just tried to update the coding standard to the latest version and got a lot of failures. I think that's not a good option.

Is there any way to run the phpcs tests on top of php7.3, while the minimum required PHP version for this package is 7.4?

ihor-sviziev commented 2 years ago

Great! I came to the conclusion that the only way to fix current tests - is to remove ignore platform requirements - it will install version laminas-code version 4.4.0 that works fine with php 8.1. This is currently impossible because the latest version of laminas-code 4.4.3 still does not support PHP 8.1. Once stable laminas-code will be released - we can remove the ignore platform requirements and test failure will be fixed.

Another option - increase the minimum required PHP version to PHP 7.4. I tried to go this way, but it started running phpcs tests with a newer version and started failing. Later, I tried to update the laminas-coding-standards - the PHP warnings and errors got fixed, but the tests themselves started failing as it includes a different set of rules, so we have to port https://github.com/laminas/laminas-server/pull/7. Not sure if it's a good idea to add so many changes in order to add PHP 8.1 support.

froschdesign commented 2 years ago

@ihor-sviziev The maintainer of this package is @arueckauer, I have added him for a review.

Thanks for the help so far! 👍

Ocramius commented 2 years ago

We should pin laminas-code to a newer version

samsonasik commented 2 years ago

Looking at the error at php 8.1 rc3, It seems you can add: #[\ReturnTypeWillChange] on the count() method.

ihor-sviziev commented 2 years ago

@samsonasik it did the trick! Thank you! A bit later I'll squash all changes

froschdesign commented 2 years ago

@arueckauer The branch for 2.11.x is missing: https://github.com/laminas/laminas-server/branches