Update PHP supported version

maxime-rainville commented 2 years ago

PHP 8.1 has been released in December. Our current constraint don't stop you from installing Silverstripe CMS on PHP8.1 but we don't know for sure everything works because we don't currently run unit test for it. PHP7.3 has also been EOL.

Acceptance criteria

[ ] Travis builds are run on PHP 8.1
[ ] Constraint on the next release branches require PHP7.4 or greater
[ ] ~~Entire silverstripe/recipe-kitchen-sink is upgraded to 8.1 compatibility~~
[ ] The Travis PHP8.1 build allows failure.
[ ] ~~References to Serializable are removed~~
- [ ] All serialize/unserialize methods are marked as @deprecated and reference matching __serialize/__unserialize methods.

Stretch goal

[ ] ~~PHP7.3 builds still run on older releases~~

PRs

obj63mc commented 2 years ago

I have done some internal testing on all of the sites we manage on SS4.10 with PHP8.1, everything runs properly but there are a ton of deprecation notices due to PHP8.1 not allowing null being passed to non-nullable internal functions. There are also some more deprecation notices around wrong return types being returned.

https://wiki.php.net/rfc/deprecate_null_to_scalar_internal_arg

When setting your environment type to 'test' or 'live' these are hidden but it looks like a lot of core code is going to need to be updated.

Some example notices that we are seeing are -

ERROR [Deprecated]: file_exists(): Passing null to parameter #1 ($filename) of type string is deprecated
IN GET /
Line 422 in /vendor/silverstripe/framework/src/Core/Injector/Injector.php

ERROR [Deprecated]: Return type of SilverStripe\Admin\CMSMenu::getIterator() should either be compatible with IteratorAggregate::getIterator(): Traversable, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice
IN GET /
Line 419 in vendor/silverstripe/admin/code/CMSMenu.php

ERROR [Deprecated] Return type of SilverStripe\ORM\DataList::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice
GET /
Line 879 in vendor/silverstripe/framework/src/ORM/DataList.php

emteknetnz commented 2 years ago

List of deprecations in 8.1 - https://php.watch/versions/8.1 (scroll to bottom of doc)

I've got a draft PR up that's a work in progress

I was initialially worried about the Serializable being deprecated, in particular if any sites had custom code that saved serialized classes to the database, rather than just to the /tmp/silverstripe-cache-php cache, where a new dir would be created on php upgrade. The worry was because our existing implementations uses json_encode, rather than and assoc_array.

I've written this test script that demonstrates that old persisted data will fallback to the old unserialise() implementations if needed

The recommended solution is to add the new serialize() and unserialize() magic methods (and optionally remove the Seraliaze interface as we're during php7.3 support.

class Test implements Serializable{

    public function __serialize(): array {}
    public function __unserialize(array $data): void {}

    public function serialize(): array {}
    public function unserialize(string $data): void {}

}

```php
<?php

class MyOld implements \Serializable
{
    private $a = '';
    private $b = '';

    public function __construct($a, $b)
    {
        $this->a = $a;
        $this->b = $b;
    }

    public function serialize()
    {
        var_dump(__FUNCTION__);
        return json_encode([$this->a, $this->b]);
    }

    public function unserialize($data)
    {
        var_dump(__FUNCTION__);
        list($this->a, $this->b) = json_decode($data);
    }
}

class MyNew
{
    private $a = '';
    private $b = '';

    public function __construct($a, $b)
    {
        $this->a = $a;
        $this->b = $b;
    }

    public function __serialize(): array
    {
        var_dump(__FUNCTION__);
        return [
            'a' => $this->a,
            'b' => $this->b
        ];
    }

    public function __unserialize(array $data): void
    {
        var_dump(__FUNCTION__);
        $this->a = $data['a'];
        $this->b = $data['b'];
    }
}

$old = new MyOld('123', '456');
$s_old = serialize($old);

$new = new MyNew('123', '456');
$s_new = serialize($new);

var_dump($s_old);
var_dump($s_new);

Running this in PHP8.1 outputs:

PHP Deprecated:  MyOld implements the Serializable interface, which is deprecated. Implement __serialize() and __unserialize() instead (or in addition, if support for old PHP versions is necessary) in /var/www/test.php on line 3

Deprecated: MyOld implements the Serializable interface, which is deprecated. Implement __serialize() and __unserialize() instead (or in addition, if support for old PHP versions is necessary) in /var/www/test.php on line 3
string(9) "serialize"
string(11) "unserialize"
object(MyOld)#2 (2) {
  ["a":protected]=>
  string(3) "123"
  ["b":protected]=>
  string(3) "456"
}
string(11) "__serialize"
string(13) "__unserialize"
object(MyNew)#4 (2) {
  ["a":protected]=>
  string(3) "777"
  ["b":protected]=>
  string(3) "888"
}
string(11) "unserialize"
object(MyNew)#5 (2) {
  ["a":protected]=>
  string(3) "123"
  ["b":protected]=>
  string(3) "456"
}

emteknetnz commented 2 years ago

Sample rector.php file used to help with conversion - see https://github.com/rectorphp/rector

Note: on framework it hangs about 90% through the process. Also on a run on assets it did not detect everything File.php:1336 was missed. So it seems OK to run this, though it's not going to fix everything.

Needs to run in php8.0 (or less), or possibly php8.1 with deprecation warnings turned off

<?php

// rector.php
use Rector\Core\Configuration\Option;
use Rector\Core\ValueObject\PhpVersion;
use Rector\Php81\Rector\FuncCall\NullToStrictStringFuncCallArgRector;
use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;

return static function (ContainerConfigurator $containerConfigurator): void {
    $parameters = $containerConfigurator->parameters();

    // paths to refactor; solid alternative to CLI arguments
    $parameters->set(Option::PATHS, [__DIR__ . '/vendor/silverstripe/framework/src']);

    // is your PHP version different from the one your refactor to? [default: your PHP version], uses PHP_VERSION_ID format
    $parameters->set(Option::PHP_VERSION_FEATURES, PhpVersion::PHP_81);

    // // Path to phpstan with extensions, that PHPStan in Rector uses to determine types
    // $parameters->set(Option::PHPSTAN_FOR_RECTOR_PATH, getcwd() . '/phpstan-for-config.neon');

    // register single rule
    $services = $containerConfigurator->services();
    $services->set(NullToStrictStringFuncCallArgRector::class);
};

GuySartorelli commented 2 years ago

related: silverstripe/silverstripe-staticpublishqueue#135 (since this is a supported module)

maxime-rainville commented 2 years ago

Merged most of the thing I could ... I think we're missing a framework PR.

maxime-rainville commented 2 years ago

Got a work in progress PR to try to squash all the warnings: https://github.com/silverstripe/silverstripe-framework/pull/10237.

Probably not necessary to get that other PR over the line to close this card.

emteknetnz commented 2 years ago

Framework PR was merged - was linked above (2nd from top) - https://github.com/silverstripe/silverstripe-framework/pull/10232

maxime-rainville commented 2 years ago

https://github.com/silverstripe/silverstripe-framework/issues/10250 will track follow up work to squash all the warnings.

maxime-rainville commented 2 years ago

https://github.com/silverstripe/silverstripe-framework/issues/10250 will track follow up work to squash all the warnings.

silverstripe / silverstripe-framework

Update PHP supported version #10219

Acceptance criteria

Stretch goal

PRs