Switch closure serializer for PHP 8.1 support #808

shadowhand · 2022-03-09T14:46:20Z

Fixes #791

shadowhand · 2022-03-09T14:50:14Z

src/Compiler/Compiler.php

        $wrapper = new SerializableClosure($closure);
-        $reflector = $wrapper->getReflector();
+        $reflector = new ReflectionClosure($closure);


Ah, it looks like #793 identified that laravel/serializable-closure only supports PHP >= 7.4, and this package supports PHP >= 7.2, so we would need to have both the Laravel package and opis/closure installed. 😢

Maybe it is time for PHP-DI to require supported PHP versions?

The alternative I guess is for the project to maintain a compatible fork of opis/closure. It'd be good to have some guidance of what the project maintainers envisage as a solution, so we can find a way forward.

I don't feel like that's necessary, given that Laravel has already relived this.

@mnapoli what is the likelihood that there can be a new major version of PHP-DI?

Even if we don't get a major version bump for PHP-DI that only supports PHP 7.4+, I think that #793 is a fine interim solution to this problem. Yes, both closure packages would be installed, but in my opinion that's much more palatable than than getting spammed with deprecation messages. As it stands, PHP-DI is so noisy that I have to @ suppress it, which makes it a liability in my codebase.

Hopefully we can get some movement on this issue from the maintainer, and thanks to those who have offered solutions! Appreciate you all!

Right, I've tried reopening #793 against master (now that there are stable versions), let's see if the tests pass: #810

Even if we bump the minimum requirement of PHP-DI to 7.3, we still need to use opis/closure for PHP 7.3. This is because laravel/serializable-closure really does require PHP 7.4, but allows it to be installed (but not used) in PHP 7.3 (see laravel/serializable-closure#31). Therefore, I think we really do need the solution provided by #793 / #810, but without PHP 7.2.

(And here is the requirement specifically in the code: https://github.com/laravel/serializable-closure/blob/bfbc6c8e85240d08d8324be3eae13b7563224a91/src/SerializableClosure.php#L28-L30)

Ahh good point! Sorry, following this issue across months with 5 minute attention span is hard 😅 thanks for the reminder.

OK, so we need a combination of #793 + this PR + drop PHP 7.2 (and keep 7.3). @shadowhand are you able to do this in this PR?

@mnapoli on it.

@mnapoli there's another issue though, ocramius/proxy-manager can't be installed on PHP 8.1 because it version locks PHP as ~8.0.0 as of the latest version 2.14.1 and there is no (released) version that supports PHP 8.1. I can work around it, but I am mentioning it.

ocramius/proxy-manager can't be installed on PHP 8.1 because it version locks PHP as ~8.0.0 as of the latest version 2.14.1 and there is no (released) version that supports PHP 8.1.

Right yes, that's why I think it's OK if we don't run CI on 8.1 (unless you have a better idea in mind)

shadowhand · 2022-03-17T18:45:38Z

I still don't love this. I hope that PHP-DI 7.0 can land soon.

shadowhand · 2022-03-18T13:31:32Z

@mnapoli test errors should be fixed now.

shadowhand · 2022-03-18T13:32:11Z

src/Definition/Resolver/ArrayResolver.php

@@ -42,7 +42,7 @@ public function resolve(Definition $definition, array $parameters = []) : array
        $values = $definition->getValues();

        // Resolve nested definitions
-        array_walk_recursive($values, function (&$value, $key) use ($definition) {
+        array_walk_recursive($values, function (& $value, $key) use ($definition) {


composer format-code changed this.

Fixes #791

shadowhand · 2022-03-18T14:44:34Z

composer.json

    },
    "require-dev": {
        "phpunit/phpunit": "^8.5|^9.0",
        "mnapoli/phpunit-easymock": "^1.2",
        "doctrine/annotations": "~1.2",
-        "ocramius/proxy-manager": "^2.0.2",
+        "ocramius/proxy-manager": "^2.2",


Changed this to fix this test error, per zendframework/zend-stdlib#97.

ocramius/proxy-manager 2.2.0 requires zendframework/zend-code (^3.3.0)

I don't know what to do about this test error, because all versions of zendframework/zend-eventmanager allow for version ^2.7 || ^3.0 of zend-stdlib, and newer versions of proxy-manager require php ^7.4. There is simply no way to resolve the build error while using --prefer-lowest.

shadowhand · 2022-03-18T14:45:34Z

@mnapoli updated to fix newest build error.

mnapoli · 2022-04-09T16:43:32Z

Since we're stuck with PHP 7.3 support and this has been the case for a while, let's jump the gun and support PHP 7.4+ ?

I've implemented that in #811 based on this PR.

mnapoli · 2022-04-09T16:46:54Z

Thanks!

netzhuffle · 2022-04-11T15:26:35Z

@mnapoli @shadowhand This Pull Request changed change-log.md, which was unmaintained for quite a while. Thus the version number mentioned there for this change (6.1.0) does not match reality (6.4.0).

I’d suggest either removing this file completely, if maintaining in the future is not planned, or fixing this issue and filling the gap for the versions in between.

mnapoli · 2022-04-11T16:02:34Z

Good point, PR welcome (I'm at a conference + launching a new major thing this week)

netzhuffle · 2022-04-11T16:04:15Z

@mnapoli PR in which direction: Removing the change log or bringing it back up to date?

shadowhand · 2022-04-11T16:09:04Z

I don't have a vote, but my vote is to bring the changelog up to date. Services like Dependabot use it.

mnapoli · 2022-04-11T16:57:35Z

Ideally updating it as it's currently used in the website: https://php-di.org/change-log.html

shadowhand mentioned this pull request Mar 9, 2022

php 8.1 support with opis/closure:4.x-dev #791

Closed

shadowhand commented Mar 9, 2022

View reviewed changes

hemberger mentioned this pull request Mar 16, 2022

Makes serializable closure work with PHP 8.1 #810

Closed

shadowhand commented Mar 18, 2022

View reviewed changes

Support PHP 8.1, drop 7.2

d1996d9

Fixes #791

shadowhand commented Mar 18, 2022

View reviewed changes

mnapoli mentioned this pull request Apr 9, 2022

PHP 8.1 support by requiring PHP 7.4 or up #811

Merged

mnapoli merged commit d1996d9 into PHP-DI:master Apr 9, 2022

shadowhand deleted the fix/php-81-serializable branch April 11, 2022 16:07

takaram mentioned this pull request Jun 3, 2024

Improve CI settings #884

Merged

Uh oh!

Switch closure serializer for PHP 8.1 support #808

Switch closure serializer for PHP 8.1 support #808

Uh oh!

Conversation

shadowhand commented Mar 9, 2022

Uh oh!

shadowhand Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shadowhand commented Mar 17, 2022

Uh oh!

shadowhand commented Mar 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shadowhand commented Mar 18, 2022

Uh oh!

mnapoli commented Apr 9, 2022

Uh oh!

mnapoli commented Apr 9, 2022

Uh oh!

netzhuffle commented Apr 11, 2022

Uh oh!

mnapoli commented Apr 11, 2022

Uh oh!

netzhuffle commented Apr 11, 2022

Uh oh!

shadowhand commented Apr 11, 2022

Uh oh!

mnapoli commented Apr 11, 2022

Uh oh!

Uh oh!

shadowhand Mar 9, 2022 •

edited

Loading