laminas / laminas-stdlib

SPL extensions, array utilities, error handlers, and more
https://docs.laminas.dev/laminas-stdlib/
BSD 3-Clause "New" or "Revised" License
190 stars 40 forks source link

Provide PHP 8.1 compatibility #34

Closed matbech closed 3 years ago

matbech commented 3 years ago

This silences the deprecated warnings in PHP 8.1:

BC concerns

The PHP 8.1 implementations add typehints to signatures where they did not previously exist. In each case, these are compatible with the previous implementation, but they do pose breakage for anybody extending the classes and overriding the relevant methods. However, these would lead to deprecation warnings, and, in some cases, fatal errors under PHP 8.1.

Fixes #24

weierophinney commented 3 years ago

@matbech I'm making progress on the 8.1 compat, and should have a comprehensive patch to push tomorrow that builds on what you've done here. Thanks for getting this started!

matbech commented 3 years ago

@weierophinney Thank you for picking this up.

weierophinney commented 3 years ago

@laminas/technical-steering-committee I'd like a review on this one, as (a) laminas-stdlib is used everywhere, and (b) while I think the approach makes sense (as all potentially non-BC changes are restricted to PHP 8.1, where the code would break without the changes), it is a potential BC break. Additionally, if accepted, it will likely become the template for when we need to introduce PHP 8.1-specific functionality in other components.

nikic commented 3 years ago

The intended way to handle this problem is to annotate methods with the #[ReturnTypeWillChange] attribute, and then only change the signatures when you are able to (i.e. your next major version).

What is the motivation for not following that approach, and duplicating these class declarations instead? This seems like a lot of complication for unclear gain.

(PHP 8.1 deprecates the ability to define extensions of internal classes that change signatures in any way, even if they fulfill the contravariance/covariance rules.)

I don't get what this refers to. Extensions of internal classes should be following normal variance rules.

weierophinney commented 3 years ago

The intended way to handle this problem is to annotate methods with the #[ReturnTypeWillChange] attribute, and then only change the signatures when you are able to (i.e. your next major version).

What is the motivation for not following that approach, and duplicating these class declarations instead? This seems like a lot of complication for unclear gain.

It started because I was first getting CS failures (as in the tool just failed completely due to fatal errors), and then a whole bunch of new Psalm errors (due to signature differences), and then failure to run the test suite across all supported versions (due, again, to errors thrown because of deprecations being raised).

When running our test suite, we have error reporting report deprecations so we can spot issues like this early and adapt; it prevents issues later once deprecations in the engine are actually removed.

Essentially, I needed solutions that would work across all supported versions, while simultaneously ensuring that our code won't needlessly break in the future.

(PHP 8.1 deprecates the ability to define extensions of internal classes that change signatures in any way, even if they fulfill the contravariance/covariance rules.)

I don't get what this refers to. Extensions of internal classes should be following normal variance rules.

This was me misreading errors as I was trying to resolve everything; I later discovered that they do follow normal covariance/contravariance rules. So please ignore that statement.

boesing commented 3 years ago

I would prefer keeping changes at a minimum rather than duplicating code just to fulfill php upgrade requirements. if the codestyle tool fails, we might switch that rather than changing code in that way. if psalm has issues, we should find a way to make it understand annotations AND attributes (are we the only framework which uses psalm with docblock and attributes?).

So I would have expect to add a few attributes and we are ready for 8.1 but this escalated quickly. if all of this is needed to make our tools happy, thats odd

weierophinney commented 3 years ago

I would prefer keeping changes at a minimum rather than duplicating code just to fulfill php upgrade requirements. if the codestyle tool fails, we might switch that rather than changing code in that way.

I'll see if I can develop an alternative patch later today.

weierophinney commented 3 years ago

Closing in favor of #35 , which provides a simpler approach.