serializable-closure icon indicating copy to clipboard operation
serializable-closure copied to clipboard

[2.x] PHP 8.4 support

Open Jubeki opened this issue 1 year ago • 8 comments

PestPHP doesn't support PHP 8.4 yet, and can therefor not be installed without the platform-req=php+

Jubeki avatar Sep 06 '24 07:09 Jubeki

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

github-actions[bot] avatar Sep 06 '24 07:09 github-actions[bot]

I'd honestly like to wait until it does support php 8.4 so we don't have to use platform-req=php

driesvints avatar Sep 06 '24 07:09 driesvints

Yeah that's why I left it in Draft for now. Just wanted to see if something else's needed to be fixed.

Jubeki avatar Sep 06 '24 07:09 Jubeki

@Jubeki I made a draft PR #91 just to experiment around. I closed it once I'm done with testing and applies the changes here.

crynobone avatar Sep 20 '24 03:09 crynobone

CleanShot 2024-09-25 at 12 55 04

I'm curious what would cause this error on PHP 8.4 tests https://github.com/laravel/framework/actions/runs/11024490854/job/30617746097

crynobone avatar Sep 25 '24 04:09 crynobone

@crynobone maybe this is a little helpful (I don't know much about serialising closures yet)

Following Code snippet causes the error in the test ImplicitRouteBindingTest::test_it_can_resolve_the_implicit_backed_enum_route_bindings_for_the_given_route (first one in your image): https://github.com/laravel/framework/blob/b17b2bd8ebcb850a909fd046c380d075fd2f118b/src/Illuminate/Routing/RouteSignatureParameters.php#L19-L23

This is the value of $action['uses'] for

PHP 8.3:

O:55:"Laravel\SerializableClosure\UnsignedSerializableClosure":1:{s:12:"serializable";O:46:"Laravel\SerializableClosure\Serializers\Native":5:{s:3:"use";a:0:{}s:8:"function";s:114:"function (\Illuminate\Tests\Routing\CategoryBackedEnum $category) {\n
            return $category->value;\n
        }";s:5:"scope";s:49:"Illuminate\Tests\Routing\ImplicitRouteBindingTest";s:4:"this";N;s:4:"self";s:32:"00000000000001060000000000000000";}}

PHP 8.4:

O:55:"Laravel\SerializableClosure\UnsignedSerializableClosure":1:{s:12:"serializable";O:46:"Laravel\SerializableClosure\Serializers\Native":5:{s:3:"use";a:0:{}s:8:"function";s:123:"function (\{closure:Illuminate\Tests\Routing\CategoryBackedEnum $category) {\n
            return $category->value;\n
        }";s:5:"scope";s:49:"Illuminate\Tests\Routing\ImplicitRouteBindingTest";s:4:"this";N;s:4:"self";s:32:"00000000000001060000000000000000";}}

See the small difference s:114 -> s:123? and function (\Illuminate\Tests\Routing\CategoryBackedEnum $category) -> function (\{closure:Illuminate\Tests\Routing\CategoryBackedEnum $category)

Not sure if this helpful, but maybe a starting point for further debugging.

Jubeki avatar Sep 25 '24 06:09 Jubeki

@crynobone I think this PR is related:

  • https://github.com/php/php-src/pull/13550

How to fix this:

https://github.com/laravel/serializable-closure/blob/43244bfd7161fdbce7419c76f9a776b7d705b333/src/Serializers/Native.php#L140-L142

Instead use the following:

$code = $reflector->getCode();
$code = str_replace('\{closure:', '', $code);

In the end, I think this is a bug with PHP and should probably be reported.

Jubeki avatar Sep 25 '24 08:09 Jubeki

I submitted an issue to php-src and they changed the return value of ReflectionFunction::getNamespaceName(). https://github.com/php/php-src/issues/16122

Here's a sample code of how the reflection methods related to closure namespace work in PHP8.4. https://3v4l.org/srIno/rfc

Now ReflectionFunction::getNamespaceName() returns "" for anonymous functions. $reflection->getClosureScopeClass()->getNamespaceName() will still work only if the closure is created in a method. Otherwise the namespace seems unable to be retrieved in a simple way.

takaram avatar Oct 01 '24 16:10 takaram

@nunomaduro do you have an idea, why for example the enum/class P\Tests\SerializerGlobalEnum does not exist? Could this have something to do with the changes to PHP 8.4 regarding Reflection? (Not sure what exactly was changed)

Pest correctly detects the defined classes / enums in the other PHP versions, but somehow not in PHP 8.4.

Screenshot 2024-10-24 at 14 52 18

Jubeki avatar Oct 24 '24 12:10 Jubeki

@Jubeki Can you rebase this to develop branch.

crynobone avatar Nov 07 '24 23:11 crynobone

@crynobone rebase was somehow annoying, I merged develop into this branch.

Alls Tests passing 😄

Jubeki avatar Nov 08 '24 07:11 Jubeki