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

[5.5] Resolve issue with queuable notifications #22275

Merged
merged 1 commit into from
Dec 4, 2017
Merged

Conversation

brendt
Copy link
Contributor

@brendt brendt commented Dec 1, 2017

This PR fixes the error reported by several people after a change in #22119

@brendt
Copy link
Contributor Author

brendt commented Dec 1, 2017

Maybe @aaronhuisinga and/or @stevebeyatte who reported having issues with #22119 could take a look if this fix solves their use case?

@aaronhuisinga
Copy link

This fixes the issue for me.

@taylorotwell
Copy link
Member

How does this actually affect anything? All you did was get rid of a temporary variable.

@brendt
Copy link
Contributor Author

brendt commented Dec 1, 2017

The temporary variable calls new $value->class which throws this exception when serialising queueable notifications.

Symfony\Component\Debug\Exception\FatalThrowableError · Class name must be a valid object or a string". Thrown from Illuminate/Queue/SerializesAndRestoresModelIdentifiers.php on line 51

I restored it to the way it was before my previous PR, whilst keeping the added functionality.

A better solution seemed to write a more explicit check:

if (class_exists($value->class)) {
    $model = new $value->class() //... ;
}

But this seemed more intrusive, so I opted for the way it was in 5.5.21. If you'd like a more explicit condition, I'm more than happy to make the changes and add a test or two.

@Daursu
Copy link

Daursu commented Dec 1, 2017

The issue occurs when trying to serialize an empty collection. Can you add a test on this so it's covered for future changes.

// ModelSerializationTest.php

    /** @test */
    public function it_serializes_an_empty_collection()
    {
        $serialized = serialize(new ModelSerializationTestClass(
            new \Illuminate\Database\Eloquent\Collection([])
        ));
        unserialize($serialized);
    }

@brendt
Copy link
Contributor Author

brendt commented Dec 2, 2017

Thanks @Daursu , added your test.

The question still stands if it would be a better idea to make the check before making a model based on $value->class more explicit by using class_exists .

@Daursu
Copy link

Daursu commented Dec 3, 2017

An empty collection will always have $value->class = null, but $value->id = [] will always be an array.

The code looks fine as it is now, and it preserves the intended behaviour in #22119.

@taylorotwell taylorotwell merged commit 145af22 into laravel:5.5 Dec 4, 2017
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.

4 participants