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] Serialize relationships #21191

Closed
wants to merge 4 commits into from
Closed

[5.5] Serialize relationships #21191

wants to merge 4 commits into from

Conversation

browner12
Copy link
Contributor

Previous attempt: #20602

Currently when you serialize a model (usually to place on a queue), and then unserialize, the model is reloaded fine, but the relationships are not. My previous solution was to have the user define on the queued object which relationships to reload. Taylor wanted something more automatic, so this PR will examine the model while it is being serialized and add the currently loaded relationships to the ModelIdentifier. Then, when the object is unserialized, will automatically reload all of the previously loaded relationships.

Some things to consider:

  • Is there a situation where the user would not want to reload relationships? If so, do we provide a toggle for them to turn it off?
  • I did not restore Collection relationships in this PR. I want to make sure this works and is accepted first, and then go back to add. Should be straightforward since all of the pieces are already there.
  • Do I need to make the $relations parameter of the ModelIdentifier constructor optional? If I don't, am I technically making a BC break on a minor release?

Thanks!

- add new `getQueueableRelations` method to queuable interfaces
- accept the 'relations' into the `ModelIdentifier`
- implement `getQueueableRelations` method on Eloquent Collection and Model.
- extract a method on `SerializesAndRestoresModelIdentifiers` for `restoreModel`. Load the relationships in this method.
- add test
@deleugpn
Copy link
Contributor

What is the goal for this? What's wrong with lazy loading relationships in the queue?

@browner12
Copy link
Contributor Author

When you are dealing with the models in PHP, it is fine.

This problem arises when you are using a service like Pusher to interact with your models in your Vue/JS code.

For example, when I pushed an Order onto the queue, and broadcast it, my Vue code that listens for it is only able to see the Order data on the other side, and can't drill down into the Lines.

@deleugpn
Copy link
Contributor

I see. Nice

@mathieutu
Copy link
Contributor

Hi! This is not exactly the same thing but I've made a package to import and export a model and it's relationships from/to json.
It can guess all the relationships and parameters to handle.

You can find it at https://github.com/mathieutu/laravel-json-syncer.

Tell me if I can help with that! 😉

@tillkruss tillkruss changed the title Serialize relationships [5.5] Serialize relationships Sep 14, 2017
@phroggyy
Copy link
Contributor

@browner12 since you're asking: yes, it's a BC break. Changing the method signature is a BC break, so if adding a new parameter to a constructor, it would need to target 5.6, or give the parameter a default. You can then make a second PR to master to remove the default parameter.

@taylorotwell
Copy link
Member

Yeah, as it stands now we would have to target this to 5.6. I think it's worth of having so probably worth re-targeting.

@taylorotwell
Copy link
Member

Can you re-submit to master branch?

@browner12
Copy link
Contributor Author

resubmitted to master in #21229.

I'd still like to get it into 5.5 if possible. It might be a little ugly, but I think it's worth it. I think the following would be the necessary steps:

  • make the $relations property on the ModelIndentifier constructor optional
  • remove the new methods on the QueuableCollection and QueueableEntity contracts
  • only use getQueueableRelations() method in SerializesAndRestoresModelIdentifiers if they are callable.
  • in Model.php, add an additional check to make sure getQueueableRelations() is callable for our QueueableCollection and QueueableEntity checks.

thoughts?

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.

None yet

6 participants