search

phpro / grumphp

A PHP code-quality tool
MIT License
4.14k stars 429 forks source link

PHP 8.1 compatibility #957

Closed veewee closed 2 years ago

veewee commented 2 years ago
Q A
Version vendor/bin/grumphp -V
Bug? no
New feature? no
Question? no
Documentation? no
Related tickets

We added PHP 8.1 compatibility in #956. However, not all downstream packages are fully compatible yet. Therefore, you might still see some deprecation warnings. The tool itself should be working though!

Awaiting deps:

Awaiting dev deps

Awaiting shim deps (patched locally)

Broken Tests

Carpenter0100 commented 2 years ago

Hi, thank you for this. Is it possible to update "psr/container" too?

dvdknaap commented 2 years ago

@veewee please also add psr/container there are packages that can't be upgrade because grumphp is still using v1

veewee commented 2 years ago

Idd surely accept a PR for that.. care to give it a try?

dvdknaap commented 2 years ago

Idd surely accept a PR for that.. care to give it a try?

Please check #968

ctrl-f5 commented 2 years ago

Are you guys willing to switch to the https://github.com/laravel/serializable-closure fork?

Since I agree with the remarks on opis/closure that depending on FFI is not a very good idea.

veewee commented 2 years ago

I am willing to switch to whatever works best. What is the reason that depending on FFI is not a good idea? Is FFI enabled by default on CLI or opt-in? However, amphp/parallel-functions is still using opis/closure, so I don't know if we can rule it out completely:

https://github.com/amphp/parallel-functions/blob/master/composer.json#L29

Lets keep an eye on this issue as well: https://github.com/amphp/parallel-functions/pull/29

tm1000 commented 2 years ago

@veewee there are some remarks in the opis/closure thread that FFI is also very slow

See: https://github.com/opis/closure/issues/102#issuecomment-919341805

Using laravel/serializable-closure would at least fix the deprecation notices right now and you can look into going to opis/closure in the future.

veewee commented 2 years ago

@tm1000 it's not that simple ... laravel/closure is not marked as a replacement for opis/closure. See composer.json So even if we switched to laravel/closure, it 'll still mean that amp/parallel-functions would have to use that same package internally. Since the namespace changed, it cannot be marked as a replacement anyways.

So I am waiting to see what AMP is going to do at this moment.

leonexcc commented 2 years ago

amphp/parallel-functions released a version 1.1.0 with laravel/closure. This now breaks grumphp on PHP 8.1 with:

Uncaught Amp\Serialization\SerializationException in worker with message "The given data could not be serialized: Laravel\SerializableClosure\Support\ReflectionClosure::__construct(): Argument #1 ($closure) must be of type Closure, Opis\Closure\SerializableClosure given, called in .../vendor/laravel/serializable-closure/src/Serializers/Native.php on line 300" and code "0"; use Amp\Parallel\Worker\TaskFailureException::getOriginalTrace() for the stack trace in the worker

veewee commented 2 years ago

Nice! Thanks for mentoning @leonexcc.

Someone interested in implementing the Laravel serializable closures in here? I'm a bit short on time ATM, so it might take a while from my side.

AegirLeet commented 2 years ago

Also broken on PHP 8.0. Pinning amphp/parallel-functions to v1.0.0 fixes it for now.

janvernieuwe commented 2 years ago

Also broken on php7.4 since parallel function 1.1.0, pinning does fix it there too

veewee commented 2 years ago

Locked it to 1.0 for 1.7.1. We can launch the compatibility with the new larevel version in grumphp v1.8 - but that will fix composer update issues for now

mougrim commented 2 years ago

Hi! Could somebody do same fix as https://github.com/phpro/grumphp/pull/981 and https://github.com/phpro/grumphp/pull/982 for grumphp v1.5.0?

Grumphp 1.7.1 requires php8.0, bug php7.4 is still supported (have security fixes).

Or say, how to do it correctly (I see here only master branch).

veewee commented 2 years ago

@mougrim : I also patched this locked version to v1.5.1

veewee commented 2 years ago

FYI : found a solution for using PHP 81 compatible laravel closures. Will release it soon. Feel free to test it out on master!

First trying to get box to work as well for grumphp-shim, since it now is PHP 81 compatible. More info: https://github.com/humbug/php-scoper/issues/642

veewee commented 2 years ago

Released the fixes for the serializable closure https://github.com/phpro/grumphp/releases/tag/v1.8.0

danepowell commented 2 years ago

I'm still seeing this using v1.5.1 on PHP 7.4:

PHP Deprecated: GrumPHP\Collection\FilesCollection implements the Serializable interface, which is deprecated.

I presume this is fixed in v1.8.1, but can we get this backported to a version compatible with PHP 7.4 as well?

veewee commented 2 years ago

@danepowell I'm a bit confused here. That interface is deprecated in php 8.1. Since you are on 7.4, you shouldnt be getting that deprecation warning?

danepowell commented 2 years ago

No problem, I should have explained. We use grumphp in a packaged application that must run on any supported PHP version (7.4-8.1). Thus we need to use grumphp v1.5.1, as it's the latest to support PHP 7.4. But if a user on PHP 8.1 runs our app, they get the deprecation errors.

Specifically, we use Composer's platform config to mandate support of PHP 7.4 in our composer.json.

veewee commented 2 years ago

@danepowell I'm currently not having the time to backport these changes myself. Feel free to provide a CR covering all supported php versions in the grumphp 1.5 range.

If your interested, I can setup a 1.5.x branch to point your commits to.

danepowell commented 2 years ago

Is there any reason that PHP 7.4 compatibility was explicitly dropped in https://github.com/phpro/grumphp/pull/956? As far as I can tell, the code in master should be cross-compatible with PHP 7.4 and PHP 8 except for the explicit Composer php dependency.

Restoring PHP 7.4 compatibility in HEAD seems much more maintainable than backporting PHP 8 to v1.5.1, which essentially becomes a forked and non-LTS branch.

Opened https://github.com/phpro/grumphp/pull/994 to bring back PHP 7.4 in HEAD.

veewee commented 2 years ago

We usually support latest 2 versions. That makes the dependency tree easier to maintain. (And less time consuming) That means lowest supported php version is in security update only mode, which is considered not really that supported in my book.

For older php versions you can then use older grumphp versions without any issue.

But indeed , if you have to support all active php versions in projects and want to use grumphp, that's an issue.