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.

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

Update PHP supported version #10219

Closed
79 of 86 tasks
maxime-rainville opened this issue Jan 28, 2022 · 9 comments
Closed
79 of 86 tasks

Update PHP supported version #10219

maxime-rainville opened this issue Jan 28, 2022 · 9 comments

Comments

@maxime-rainville
Copy link
Contributor

maxime-rainville commented Jan 28, 2022

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

obj63mc commented Feb 1, 2022

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

emteknetnz commented Feb 2, 2022

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

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);
};

This was referenced Feb 9, 2022
@GuySartorelli
Copy link
Member

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

@maxime-rainville
Copy link
Contributor Author

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

@maxime-rainville
Copy link
Contributor Author

Got a work in progress PR to try to squash all the warnings: #10237.

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

@emteknetnz
Copy link
Member

Framework PR was merged - was linked above (2nd from top) - #10232

@maxime-rainville
Copy link
Contributor Author

#10250 will track follow up work to squash all the warnings.

1 similar comment
@maxime-rainville
Copy link
Contributor Author

#10250 will track follow up work to squash all the warnings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants