search

silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
722 stars 821 forks source link

PHP 8.1 compatibility #10250

Closed emteknetnz closed 2 years ago

emteknetnz commented 2 years ago

Split off from https://github.com/silverstripe/silverstripe-framework/issues/10219

Upgrade all the support modules to support php 8.1

Agreed way forward to resolve common errors https://github.com/silverstripe/silverstripe-framework/pull/10237#issuecomment-1058557519

Reference to some breaking changes in php 8.1 - https://stitcher.io/blog/new-in-php-81#breaking-changes

Note

Steve created some science experiment to add a _a method that can note docblock/parameter type mismatched and a _c method that can recast variable to their expected type. He also has a script to apply it to the entire code base.

Update: this approach won't work for php 8.1. Reason being that the _a() method profiling will certainly detect the say all the string and null arguments being passed in for a param, we cannot determine whether or not null is valid in this instance or not, so we don't know if _c('string', $myvar, 1) should be included or not

Acceptance criteria

Explanation of the conversion process in the PRs below

Because this is an automated process this certainly goes on the side of over converting code. If we move to full-type hinting param and return type in the future, we could run another code sweep to remove a bunch of the code conversions. Currently we cannot rely on the docblock types as they are very unreliable.

There are also #[\ReturnTypeWIllChange] method attributes added to class methods where relevant, as well as some other methods that are updated, such as adding __serialized()/__unserialized()

Current PHP 8.1 unit test status

Follow up PRs

PRs

GraphQL 3 PRs

"Manual fix" PRs

solr-php-client fork

Installer (all prs use null coalesce)

No changes

Kitchen-sink (all prs use null coalesce)

No changes

PRs for dependencies

emteknetnz commented 2 years ago

Honestly this is starting to look like a nightmare due to the sheer number of call to native php functions like str_replace, where suddently null values are no longer allowed

The core issue of this is that Silverstripe was originally written for PHP 5 before param types were a thing, so everything is mixed value

We basically need to cast params in a stunningly large number of places, e.g.:

silverstripe/framework Convert.php

     * @param string $path
     * @param string $separator
     * @param bool $multiple Collapses multiple slashes or not
     * @return string
     */
    public static function slashes($path, $separator = DIRECTORY_SEPARATOR, $multiple = true)
    {
        $path = (string) $path; // << we need to do this

Like wise we have the same issue with a lack of return type enforcement:

silverstripe/versioned Versioned.php

     * @return string
     */
    public static function get_reading_mode()
    {
        return self::$reading_mode;
    }

Which, despite what the docblock says, it can return null.

So any calls to this method can now fail, such as str_replace('abc', 'def', Versioned::get_reading_mode());

I think we might actually be better off being more ambitious here and converting ALL the docblock to actual param and return types. If not, were essentially stuck with the $path = (string) $path; casting of params instead.

If we're going to do all the work, I think we should do it properly, rather than this somewhat improper casting method

This would be a breaking change, though so is php 8.1

emteknetnz commented 2 years ago

For those not familiar with the issue here, when upgrading PHP 8.1 deprecation warnings are now thrown when passing null as a argument instead of the typed value. Since we cannot realistically turn of deprecation warnings on local environments, this leads to hundreds of deprecation warnings when doing local dev.

So in practice, PHP 8.1 is effectively a breaking API change itself:

PHP 8.0: strpos(?string $haystack, ?string $needle, ?int $offset = 0): int|false

PHP 8.1: strpos(string $haystack, string $needle, int $offset = 0): int|false

Options here are: a) Use param casting method $myvar = (string) $myvar; and release as a minor of 4 b) Use param types which is a big API change, and release as 5.0 (new major) c) Use param types which is a big API change, and release as a new minor of 4 (semver violation) - with a very large note in the changelog saying there's a high chance you'll get regressions, so really do test this upgrade

@silverstripe/contributing-committers @maxime-rainville

michalkleiner commented 2 years ago

I'm for B) for the sole reason of (possibly) starting to release major versions faster so that we don't have to put major changes on the back burner for months or years until 5 finally comes. C) feels risky as it may have impacts you wouldn't expect from a minor version change even with a huge note in the changelog. A) is just obscuring the problem and letting it come back at us later, or creating a technical debt that will be hard to repay.

kinglozzer commented 2 years ago

I think we’d need to carefully consider the implications of jumping into a 5.x release for this. If we want to get away from monolithic major releases then that’s great, but we need to consider the implications for things like modules, support timelines and community expectations about what a major release is. I don’t think any of those challenges are insurmountable of course, but if a 5.x release is suddenly on the table then it should probably be part of a wider discussion around versioning/releases. If we do decide to take that path then we’d need to decide which breaking changes to include - just the PHP compatibility? Or include embed v4 in that release instead of 4.x? Any other major pain points we can address at the same time? I honestly have no idea what the state of our master branch currently is, but forking a 5 branch from 4 instead of master is an option in the short-term.

Given the description of the issue in the comments above though, I don’t have any problem with option A. For core PHP functions we can cast arguments to their expected types, I can’t foresee any future problems or technical debt from that? For casting issues caused by core code, I think we just have to use our judgement. For the Versioned::get_reading_mode() example, I see two approaches:

1) We can cast the value returned anywhere we use it in core code, and users who upgrade to PHP 8.1 will have to do the same in their code (and module authors will have to do the same too) 2) We treat docblocks as the source of truth, and implement casting in core code. Whether this is a BC-break is a bit ambiguous given the docblocks, but we just have to use our judgement to figure out how likely it is to actually cause issues for people. For Versioned::get_reading_mode() my instinct is that this approach would be fine, but there may be other methods where it wouldn’t be

Also cc-ing @silverstripe/core-team because the contributing committers group is smaller 😉

emteknetnz commented 2 years ago

A) is feasible, and short term it's easier. My issues with it is that it's that it's still a significant amount of effort to implement, it's a frankly weird solution and it's kicking the can down the road

At some point we'll modernize the code and put types on everything anyway, so it seems like we should just do this step to begin with i.e. do it properly the first time around

If we do decide to take that path then we’d need to decide which breaking changes to include - just the PHP compatibility?

If we do a major I think it's important to try and limit the amount of changes we try to put in for a few reasons:

maxime-rainville commented 2 years ago

I would like us to get to a point where we can release new majors again. But that would require us to have a wider discussion around what we put in major and what kind of changes we ship in there. My preference would be to take the Symfony approach where new majors are just removing deprecated API and paying down some technical debt ... so basically, transitioning from CMS4 to CMS5 should be a much smaller step than CMS3 to CMS4.

But realistically, there's no way we could be in a position to do this before the end of this year.

So the question is do we want to have official PHP8.1 support in the 4 release line? If the answer is yes, then we have to decide between A and C. Option C seems like a bridge too far ... basically it's like releasing CMS 5, but pretending we are not.

I could live with Option A ... and we have taken this general approach in the past, but maybe we shouldn't have.

I guess there's an Option D here which would be tell people to suppress deprecation warning if they want to use PHP8.1. Maybe, that can be a transitory step to Silverstripe CMS 5.

emteknetnz commented 2 years ago

I don't really like option D. It's a similar story to how Silverstripe could actually run php 8.0 in a live environment several months before we officially announced it. We delayed announcing this because phpunit 5.7 wasn't compatibile and we didn't want to say "PHP 8.0 works" when it wasn't 100% there. Telling people they can use PHP 8.1 is just inviting confusion

But realistically, there's no way we could be in a position to do this before the end of this year.

I don't think there's a pressing need to deliver PHP 8.1 at this stage, so delaying this by say 6 months doesn't seem like that big of a deal

It's also worth remembering that a lot community module won't be PHP 8.1 compatible either, so from a practical point of view PHP 8.1 is never going to be a quick win

xini commented 2 years ago

I don't think there's a pressing need to deliver PHP 8.1 at this stage, so delaying this by say 6 months doesn't seem like that big of a deal

That's infuriating! In 6 months time PHP 8.2 will almost be out and you should be looking at updating to 8.2 release candidates at that time!

As already mentioned in https://github.com/silverstripe/silverstripe-framework/issues/10170, if you want to remove support for older PHP versions as soon as they go EOL even though there is no technical reason to do so (a.k.a 7.3), at least get onto supporting new versions as soon as they are out as well!

/rant. sorry. not.

emteknetnz commented 2 years ago

Just updating on this one, we're almost certainly not going to look at updating method signatures on this one, as the scope of the work is simply too large, and it's too hard to validate which method parameters are legitimately nullable vs being mistakenly passed null values currently

Instead we're looking to use a combination of a forked version rector to update native php function calls and something to update docblock return types (required for rector to function correctly) in order to get php8.1 compatibility within CMS4

Here's an example PR https://github.com/silverstripe/silverstripe-assets/pull/482/files

maxime-rainville commented 2 years ago

Had a look at the first 3 draft PRs ... it seems like we are systematically explicitly casting some method parameters everywhere ... even one that logically will never return a null parameter: rtrim((string) realpath((string) $filesPath).

I'm not super enthusiastic with this approach. It's going to be a lot of work to remove all those explicit cast if/when we switch to string type in a future major release. My instinct would have been to manually fix instances where null values breaks test until all the builds are green.

Maybe, there's no other way to properly solve this than explicit cast everything. Is there any intermediary solution we could look at? Maybe we go over the automated tool's output and remove the explicit cast where it is obviously clear that the value will never be null.

GuySartorelli commented 2 years ago

All linked PRs are merged and ACs are met.