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 #957

Closed
8 of 9 tasks
veewee opened this issue Nov 26, 2021 · 23 comments
Closed
8 of 9 tasks

PHP 8.1 compatibility #957

veewee opened this issue Nov 26, 2021 · 23 comments
Labels
enhancement wip

Comments

@veewee
Copy link
Contributor

veewee commented Nov 26, 2021 •
edited
Loading

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

  • Fix skipped E2E tests - currently won't do. They seem buggy edge cases anyways. If there is any need for them to still work, time can be invested
@Carpenter0100
Copy link

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

@dvdknaap
Copy link
Contributor

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

@veewee
Copy link
Contributor Author

veewee commented Dec 16, 2021 •
edited
Loading

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

@dvdknaap
Copy link
Contributor

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

Please check #968

@ctrl-f5
Copy link

ctrl-f5 commented Jan 19, 2022

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

veewee commented Jan 19, 2022 •
edited
Loading

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:
amphp/parallel-functions#29

@tm1000
Copy link

tm1000 commented Jan 26, 2022 •
edited
Loading

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

See: opis/closure#102 (comment)

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

veewee commented Jan 27, 2022 •
edited
Loading

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

leonexcc commented Feb 4, 2022

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

veewee commented Feb 4, 2022 •
edited
Loading

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

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

@janvernieuwe
Copy link
Member

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

@veewee veewee mentioned this issue Feb 4, 2022
Merged
@veewee
Copy link
Contributor Author

veewee commented Feb 4, 2022

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

@leonexcc leonexcc mentioned this issue Feb 4, 2022
Merged
@ctrl-f5 ctrl-f5 mentioned this issue Feb 4, 2022
Closed
10 tasks
@mougrim
Copy link

mougrim commented Feb 5, 2022

Hi! Could somebody do same fix as #981 and #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).

@mougrim mougrim mentioned this issue Feb 5, 2022
Merged
mougrim added a commit to mougrim/php-xdebug-proxy that referenced this issue Feb 5, 2022
@mougrim
v0.5.0
5a58996 
There are next changes:

- Minimum PHP version supported upgraded to 7.4 (#29)
- PHPUnit version upgraded to 9 (#29)
- Add run unit tests Github action (#30, #31)
- Remove travis (#30)
- Fix code style issues (#32)
- Fix by workaround grumphp issue with TypeError, see phpro/grumphp#957 (#32)
- Update friendsofphp/php-cs-fixer to 3 (#33)
- Add psalm (#34, #35)
- Add check cs github action (#36)
mougrim added a commit to mougrim/php-xdebug-proxy that referenced this issue Feb 5, 2022
@mougrim
(#37) v0.5.0
aa33b66 
There are next changes:

- Minimum PHP version supported upgraded to 7.4 (#29)
- PHPUnit version upgraded to 9 (#29)
- Add run unit tests Github action (#30, #31)
- Remove travis (#30)
- Fix code style issues (#32)
- Fix by workaround grumphp issue with TypeError, see phpro/grumphp#957 (#32)
- Update friendsofphp/php-cs-fixer to 3 (#33)
- Add psalm (#34, #35)
- Add check cs github action (#36)
@veewee
Copy link
Contributor Author

veewee commented Feb 7, 2022

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

@veewee veewee mentioned this issue Feb 7, 2022
Merged
@veewee
Copy link
Contributor Author

veewee commented Feb 7, 2022 •
edited
Loading

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: humbug/php-scoper#642

@veewee
Copy link
Contributor Author

veewee commented Feb 8, 2022

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

@danepowell
Copy link
Contributor

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

veewee commented Mar 11, 2022

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

danepowell commented Mar 11, 2022 •
edited
Loading

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

veewee commented Mar 21, 2022

@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 danepowell mentioned this issue Mar 21, 2022
Merged
10 tasks
@danepowell
Copy link
Contributor

danepowell commented Mar 21, 2022 •
edited
Loading

Is there any reason that PHP 7.4 compatibility was explicitly dropped in #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 #994 to bring back PHP 7.4 in HEAD.

@veewee
Copy link
Contributor Author

veewee commented Mar 21, 2022 •
edited
Loading

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.

@veewee veewee mentioned this issue Mar 25, 2022
Merged
@veewee veewee closed this as completed Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement wip
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
10 participants
@ctrl-f5 @tm1000 @mougrim @veewee @danepowell @dvdknaap @leonexcc @janvernieuwe @AegirLeet @Carpenter0100