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

Adds support for closure inside context with enum property #47

Merged
merged 2 commits into from
May 16, 2022

Conversation

ksassnowski
Copy link
Contributor

Fixes #43

Description

This PR fixes a bug when trying to serialize a closure that was defined inside a class where any class in its context has a property containing an enum. Since that's quite a mouthful, here's an example.

Given these enum and class declarations:

enum MyEnum
{
    case Foo;
    case Bar;
}

class MyClass
{
    public MyEnum $enum = MyEnum::Foo;
    public Closure $closure;

    public function __construct()
    {
        $this->closure = function () {
            return $this->enum;
        };
    }
}

A couple important things to point out:

  • The class MyClass has a property with an enum as its value. It's not actually important that this field exists on MyClass specifically. As long as some class somewhere in the closure's context has a property with an enum as its value, the bug will still occur. So if MyClass had a property of type MyOtherClass and MyOtherClass had a property of type MyEnum, the same thing would happen.
  • The closure it's referencing $this. This is important (maybe pun intended) because its what's triggering the call to Serializers\Native::wrapClosures.
  • The $enum field actually has a value. If this was a nullable field and it happened to be null the error would not occur.

When this closure gets serialized, the serializer recursively walks through every property of MyClass (and all of its parent classes, if applicable) and and calls wrapClosures on each of them. Once it hits the property containing the enum, it crashes with a Cannot instantiate enum MyEnum error. This is because it's trying to call ReflectionClass::newInstanceWithoutConstructor on the enum which doesn't work.

The fix

Since enums serialize just fine on their own, I fixed this by adding an additional base case to the respective elseif branch of the wrapClosures method to exclude instances of UnitEnum. The is_object call on its own is not specific enough since that returns true for UnitEnum as well.

I have added the simplest test case I could come up with to reproduce the error. I have also tested this patch on one of my current projects where I encountered this bug and everything seems to work.

@ksassnowski
Copy link
Contributor Author

@ricardoboss Since you opened the initial issue, could you verify if the code that produced the bug on your end matches the scenario I described? If not, there might be another case I haven't discovered yet 😅

@ricardoboss
Copy link

@ksassnowski I will look into it!

@nunomaduro nunomaduro requested a review from taylorotwell May 16, 2022 17:04
@taylorotwell taylorotwell merged commit 09f0e9f into laravel:master May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for PHP 8.1 enums
4 participants