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

Set Model Relationship && Fix serialization #3226

Closed

Conversation

cosmastech
Copy link
Contributor

@cosmastech cosmastech commented Mar 29, 2023

It looks like serialization/deserialization never would've worked if media was being added to a collection (without first refreshing the model).

The properties set by the InteractsWithMedia trait contain closures.

This MR add back in the set relation functionality, along with a fix for InteractsWithMedia@__sleep() so that those properties are not getting serialized. This should save on data pushed to the queue as well. 😄

freekmurze and others added 7 commits March 6, 2023 11:26

Verified

This commit was signed with the committer’s verified signature.
nflaig Nico Flaig

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@cosmastech cosmastech force-pushed the feature/set-model-relationship branch from 1463d54 to bc14fd7 Compare March 29, 2023 16:17
@cosmastech
Copy link
Contributor Author

#3224 (comment)

Thanks to @HolgerHatGarKeineNode for testing this.

@freekmurze let me know if there's anything else you'd like to see done with this PR to confirm it's working properly

@cosmastech
Copy link
Contributor Author

Closing this. The serialization issue is addressed #3258

@cosmastech cosmastech closed this May 2, 2023
@michaelbaril
Copy link
Contributor

Hi @cosmastech , I was about to create a similar PR and then I stumbled upon yours. I don't understand why you closed it though? The other PR you're referring to addresses the serialization issue, but doesn't set the model relation (which is what I'm interested in).
Thanks!

@cosmastech
Copy link
Contributor Author

cosmastech commented Jul 13, 2023

Hi @cosmastech , I was about to create a similar PR and then I stumbled upon yours. I don't understand why you closed it though? The other PR you're referring to addresses the serialization issue, but doesn't set the model relation (which is what I'm interested in). Thanks!

Hello @michaelbaril

If you're interested in adding this functionality, you can extend the MediaRepository class and bind the new implementation into the container.

This is untested, but something like:

namespace App;


use Illuminate\Database\Eloquent\Model;
use Illuminate\Support\Collection;
use Spatie\MediaLibrary\HasMedia;
use Spatie\MediaLibrary\MediaCollections\MediaRepository;

class MyMediaRepository extends MediaRepository
{
    public function getCollection(
        HasMedia $model,
        string $collectionName,
        array|callable $filter = []
    ): Collection {
        return $this->applyFilterToMediaCollection($model->loadMedia($collectionName), $filter)
            ->each(fn(Model&HasMedia $media) => $media->setRelation('model', $model));
    }
}

The binding I'm less sure about. I don't think you could put the binding within AppServiceProvider, since the MediaLibraryServiceProvider is loaded afterwards.

You could extend the package's MediaLibraryServiceProvider and override the register method so it's relying on your custom MyMediaRepository.


Another option would be to create your own InteractsWithMedia trait.

namespace App\Concerns;

use Illuminate\Database\Eloquent\Model;
use Spatie\MediaLibrary\HasMedia;
use Spatie\MediaLibrary\InteractsWithMedia;
use Spatie\MediaLibrary\MediaCollections\MediaCollection;

trait InteractsWithMediaTrait
{
    use InteractsWithMedia {
        getMedia as baseGetMedia;
    }
    
    public function getMedia(string $collectionName = 'default', array $filters = []): MediaCollection
    {
        return $this->baseGetMedia($collectionName, $filters)
            ->each(fn(Model&HasMedia $media) => $media->setRelation('model', $this));
    }
}

Then replace your models which employ InteractsWithMedia to use InteractsWithMediaTrait instead.


As for the reason I closed it... I didn't assume it was going to get merged. It broke the package for a handful of people, so I assume @freekmurze would be less inclined to merge this change in again, even after fixing the serialization issue. The benefit it provides is only if the client code is fetching the media's model, which can be worked around other ways.

Another thought I had just now is that I'm not sure how it might work in an environment like Octane tbh. Would hate for this to lead to memory leaks.

Feel free to open your own PR for this of course!

@cosmastech cosmastech deleted the feature/set-model-relationship branch July 13, 2023 10:48
@michaelbaril
Copy link
Contributor

Hey @cosmastech , thanks for your fast and detailed answer! Actually I already did it using the 2nd approach you suggest (overriding the trait), but I still thought of doing a PR because I felt it should be the standard behavior. When using $registerMediaConversionsUsingModelInstance = true, it avoids unnecessary queries.

Anyway, I'll probably open my own PR and see what the Spatie guys think.

Thanks again!

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

4 participants