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

New config option to restore default laravel model serialization behaviour #3210

Merged
merged 1 commit into from
Jul 20, 2023

Conversation

BARNZ
Copy link
Contributor

@BARNZ BARNZ commented Mar 17, 2023

Relates to dicusssion #3203

Problem

When upgrading media library from v8 to the latest version there is an inadvertent change introduced in commit 0730227 which adds a jsonSerialize function to the MediaCollection class. This overrides the default model toJson() behaviour and can result in a number of bugs for existing applications that expect the standard laravel model toJson behaviour for media models.

For example in v8 the following worked as one would expect:

Route::get('test', fn () => Media::all());

In v9/v10 this behaviour is now broken and will always return an empty array []

Below I have tested a bunch of other typical use-cases that are also affected when upgrading to v10. Basically anything that returns a Spatie\MediaLibrary\MediaCollections\Models\Collections\MediaCollection::class:

// In v9/v10 always returns []
Route::get('media-test1', fn () => Media::all());

// In v9/v10 always returns []
Route::get('media-test2', fn () => Media::take(10)->get());

// In v9/v10 always returns []
Route::get('media-test3', fn () => MyModel::first()->media);

// In v9/v10 always returns []
Route::get('media-test4', fn () => MyModel::first()->media->toJson());

// Works fine in v9/v10 - not calling into jsonSerialize
Route::get('media-test5', fn () => MyModel::first()->media->toArray());

// In v9/v10 this actually returns something but only specific set of hardcoded media fields as per MediaCollection::jsonSerialize()
// Breaks any custom model usages that may have additional media fields
// Does not respect the hidden/visible fields of any custom media model
Route::get('media-test6', fn () => MyModel::first()->getMedia());

// Works fine in v9/v10 - eagerloads give us a regular laravel collection
Route::get('media-test7', fn () => MyModel::with('media')->first());

Why is this problem not so widespread?

I suspect people have been using a manual workaround which is to call toArray() first or wrapping in a plain laravel collect(). Both these cases work fine but are cumbersome and can be difficult to track down all possible places and situations where media collections are serialized in large codebases.

// Example workarounds:
Route::get('media-test1', fn () => Media::all()->toArray());
Route::get('media-test2', fn () => collect(MyModel::first()->media);

What does this merge request do?

This merge request introduces a new config option use_default_collection_serialization which when enabled will restore the default laravel model jsonSerialize behaviour (also used in media library v8).

I have left this option as false by default so that there is no change in current v9/v10 behaviour from what the maintainers had originally planned.

@freekmurze freekmurze merged commit 47a0da5 into spatie:main Jul 20, 2023
@freekmurze
Copy link
Member

Thanks!

@shadowspade
Copy link

Thank you!

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.

3 participants