turbo-laravel icon indicating copy to clipboard operation
turbo-laravel copied to clipboard

Fix payload serialization

Open tonysm opened this issue 1 year ago • 4 comments

When we send a broadcasting job to be processed in background, because the job payload can be anything (whatever you want to pass to the view), we don't really use Laravel's ModelIdentifier class (here).

The current implementation works, but the jobs are unnecessarily big, since the object is serialized using PHP's normal serialization. This also has the side-effect of the model having all the attributes it had during from the moment the model was serialized. For instance, if we wanted to return the HTML slightly modified as an HTTP response Turbo Stream for the user that created the resource, we'd use the $model->wasRecentlyCreated, but if we wanted the broadcasted Turbo Stream to not be modified, we would have to override the data passed to the view, because since the model was serialized, it will also have the $model->wasRecentlyCreated set to true when the broadcast job is processed.

I think this could be solved by walking through the data array and finding out the eloquent models and either converting them to either use ModelIdentifier or maybe using globalid-laravel (here).

tonysm avatar Sep 03 '23 21:09 tonysm

I just tested using Laravel's SerializableClosure, but I don't think it's gonna work, unless we change the API.

For this package to work, the API would have to be something like this:

$todo->broadcastAppend()
  ->view(fn () => view('todos._todo', ['todo' => $todo]))
  ->later();

Notice the fn () => view(/** ... */) Closure passed to the view. This way, the Closure would be correctly serialized. default. This is because the serializable-closure package won't serialize it correctly like this:


$data = [
  'todo' => $todo,
];

serialize(new SerializableClosure(fn () => view('todos._todo', $data)));

We could probably handle the default behavior inside the package itself (serializing the model correctly.

Also, using the package we probably wouldn't be able to dispatch a queued job in a REPL context like tinker (or Tinkerwell), since it won't work in those envs.

tonysm avatar Sep 03 '23 22:09 tonysm

@tonysm The call to the $comment->broadcastReplace(); couldn't be moved to the job? instead of all of the parameters needed to build TurboStreamBroadcast ?

reshadman avatar Jun 11 '24 08:06 reshadman

Not sure I get your suggestion. 🤔 The issue is not about the broadcast methods themselves. The problem is only when we pass custom views with data since we need to serialize that to render on the queued job. If we're only broadcasting the model itself, then this is not an issue, since it serializes just fine.

tonysm avatar Jun 12 '24 12:06 tonysm