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.7] Fix QueueableCollection serialization of Eloquent Models when using Binary IDs #27271

Merged
merged 2 commits into from Jan 23, 2019

Conversation

Projects
None yet
2 participants
@tonysm
Copy link
Contributor

tonysm commented Jan 22, 2019

Found this issue during development where we have a model that uses binary UUIDs. While the single model serialization works fine, the collection serialization + JSON encoding breaks (UTF-8 issues).

Added a test demonstrating the problem we faced (the testJsonSerializationOfCollectionQueueableIdsWorks one) and some other testing that still works for Pivot Models as well as Eloquent Models.

tonysm added some commits Jan 22, 2019

Fix QueueableCollection relying on Pivot->getQueueableId() instead of…
… QueueableEntity->getQueueableId()

Found this issue during development where we have a model that uses binary UUIDs.
While the single model serialization works fine, the collection serialization +
JSON encoding breaks.

@driesvints driesvints changed the title Fix QueueableCollection serialization of Eloquent Models when using Binary IDs [5.7] Fix QueueableCollection serialization of Eloquent Models when using Binary IDs Jan 22, 2019

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Jan 23, 2019

I don't understand this change. We were checking for Pivot and now we're checking for something that Pivot doesn't even implement?

@tonysm

This comment has been minimized.

Copy link
Contributor Author

tonysm commented Jan 23, 2019

Pivot extends from Model, which implements that interface. However, Pivot overrides the behavior of the base Model for this particular method in the asPivot trait.

As the Eloquent Collection's getQueueableIds method is the implementation of QueueableCollection, I thought it would make sense to have it relying on the QueueableEntity (where the getQueueableId method is defined) instead of it relying in the Pivot class specifically.


The change makes sure the Collection::getQueueableIds() return the Item::getQueueableId() for anything that implements the QueueableEntity contract (as the base Model implements), not specifically the Pivot class.

@taylorotwell taylorotwell merged commit d58d20f into laravel:5.7 Jan 23, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment