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.

Sign up for GitHub

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

Jump to bottom

PHP 8.1 compatibility #10250

Closed
emteknetnz opened this issue Mar 7, 2022 · 11 comments
Closed

PHP 8.1 compatibility #10250

emteknetnz opened this issue Mar 7, 2022 · 11 comments
Assignees
@emteknetnz
Labels
affects/v4 complexity/high impact/high

Comments

@emteknetnz
Copy link
Member

emteknetnz commented Mar 7, 2022 •
edited by GuySartorelli
Loading

Split off from #10219

Upgrade all the support modules to support php 8.1

Agreed way forward to resolve common errors #10237 (comment)

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

  • PHP8.1 build are green.
  • We don't break semver.
  • The magical process for auto updating our code base is documented somewhere. (it's in this issue)
  • CI fails on php deprecation warnings
  • graphql4 is also updated
  • Ensure an installable version of php 8.1 compatible tractorcow deps is available - see ENH PHP 8.1 compatibility tractorcow/classproxy#5 (comment)

Explanation of the conversion process in the PRs below

  • Automatic code modification using https://github.com/emteknetnz/php81-fixer
  • Static code analysis that ensures any arguments passed to native php functions are not null
    - If the native param only accepts a single type of (string|bool|int|float) then it is cast i.e (string)
    - If the native param only accepts an array then it uses the ternary operator i.e. ?: []
    - If the native param accepts multiple types then it uses the ternary operator with a bias to using string i.e. ?: ''
  • Use the null coalescing operator for variables passed to native functions that do not accept null values
  • There are some conditions where arguments are not converted such as the argument is a native constant, is an inner native function call with a single return type, is using the class static property e.g. SiteTree::class

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

  • silverstripe/silverstripe-errorpage
  • silverstripe/silverstripe-siteconfig

Kitchen-sink (all prs use null coalesce)

No changes

  • content-widget
  • crontask
  • elemental-fileblock
  • security-extensions
  • taxonomy
  • dnadesign/silverstripe-elemental-userforms
  • tractorcow/silverstripe-proxy-db

PRs for dependencies
@emteknetnz emteknetnz self-assigned this Mar 7, 2022
This was referenced Mar 7, 2022
Merged
Closed
Merged
@emteknetnz
Copy link
Member Author

emteknetnz commented Mar 7, 2022 •
edited
Loading

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

@maxime-rainville maxime-rainville mentioned this issue Mar 7, 2022
Closed
86 tasks
@emteknetnz
Copy link
Member Author

emteknetnz commented Mar 7, 2022 •
edited
Loading

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member Author

emteknetnz commented Mar 8, 2022 •
edited
Loading

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:

  • The more we breaking changes we put in, the more upgrade pain experienced by end users, who will have the major upgrade forced on them rather than by choice
  • 3 to 4 was a painful experience for many due to the many breaking changes, and many site owners had a very negative perception of the value of the major upgrade. With 4 to 5 it would be good if we could basically do the opposite
  • People are leaning towards doing more frequent, smaller major upgrades, so if something misses the boat now, it's not lost forever.

@maxime-rainville
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

xini commented Mar 22, 2022

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 #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 emteknetnz mentioned this issue Mar 28, 2022
Closed
@emteknetnz
Copy link
Member Author

emteknetnz commented Mar 30, 2022 •
edited
Loading

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

This was referenced Apr 4, 2022
Merged
Merged
Merged
This was referenced Apr 13, 2022
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Closed
Merged
Merged
@emteknetnz emteknetnz removed their assignment Apr 20, 2022
@emteknetnz emteknetnz mentioned this issue Apr 20, 2022
Merged
This was referenced Apr 25, 2022
Merged
Merged
Merged
Merged
@emteknetnz emteknetnz mentioned this issue Apr 26, 2022
Merged
@GuySartorelli
Copy link
Member

All linked PRs are merged and ACs are met.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@emteknetnz emteknetnz

Labels
affects/v4 complexity/high impact/high
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@michalkleiner @xini @maxime-rainville @kinglozzer @emteknetnz @GuySartorelli