-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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] Add support for newQueryForRestoration from queues #22119
Conversation
All of a sudden, I'm getting |
Looks like something with OPCache holding partial old code and partial new code. Clearing OPCache fixes it. |
@brendt @taylorotwell it seems that this has broken some Queueable Notification classes for me. If I pass an empty Collection or Eloquent Collection as a parameter into a Queueable Notification class, I'm now seeing the Exception "Symfony\Component\Debug\Exception\FatalThrowableError · Class name must be a valid object or a string". It's thrown from Illuminate/Queue/SerializesAndRestoresModelIdentifiers.php on line 51. If I rollback to Laravel v5.5.21, this is fixed immediately. I can also convert the empty Collection to an array and it works properly. |
I see why that happens. Instead of doing this $model = (new $value->class)->setConnection($value->connection);
return is_array($value->id)
? $this->restoreCollection($value)
: $this->getQueryForModelRestoration($model, $value->id)
->useWritePdo()->firstOrFail(); It should be this return is_array($value->id)
? $this->restoreCollection($value)
: $this->getQueryForModelRestoration((new $value->class)->setConnection($value->connection), $value->id)
->useWritePdo()->firstOrFail(); Should I submit a new PR? |
+1 on this breaking queueable notifications for me as well |
I went ahead and opened a new PR with a fix. |
Breaking queues for me as well. |
Scratch that, the issue I ran into was that my supervisor wasn't automatically restarting after I pushed code. Once I restarted supervisor everything worked fine.
|
@stevebeyatte how you fix it? I have some problem in toBroadcast function. I use
on |
When queueing models, there's the
getQueueableIds
method which allows you to override the ID value which is serialised to JSON.It is however not possible to map the serialised ID back into the other direction when unserialising the model. This PR adds that method.
The need for using another ID when serialising models becomes clear when you're using binary UUIDs as primary keys. There are a few issues addressing the problem:
When serialising a model, and
json_encoding
that serialised data, JSON fails when it encounters a binary field. To solve this problem, the binary ID could. eg. be stored as a base64 encoded string. However, when unserialising the model, you'll need a way to decode that string back to its binary form, to be able to query the database.A concrete example in a model, using this functionality:
The default implementation of
newQueryForRestoration
is added in theModel
class, and keeps the default functionality.Note that by adding the
newQueryForRestoration
method, the need for theis_array($value->id)
check inSerializesAndRestoresModelIdentifiers
can easily be refactored. The check can now happen in thenewQueryForRestoration
instead. I didn't add this change though, because it would have made the diff more confusing. I'm willing to also refactor this, and add tests if needed.