spatie / laravel-medialibrary

Associate files with Eloquent models
https://spatie.be/docs/laravel-medialibrary
MIT License
5.74k stars 1.07k forks source link

Add support for callables in hasMedia() filters #3696

Open crossiatlas opened 1 month ago

crossiatlas commented 1 month ago

Description of Changes I encountered a scenario where I expected $model->hasMedia('my-collection', $filters) to accept a closure for $filters, but it's typehinted to only support an array. 2024-08-20_11-26

This PR updates the typehint to match getMedia()

In the meantime I worked around this via:

$model->getMedia('my-collection', fn (Media $media) => isset($media->custom_properties['something'])))->isNotEmpty()

Changes Made

Resolves #3526

(NOTE: This is my first contribution to this repository, I believe I've met the requirements but feel free to let me know if there's anything I'm missing and I'll update my PR. Thanks!)

freekmurze commented 1 month ago

Could you also update the docs with an example?

crossiatlas commented 1 month ago

@freekmurze Absolutely!

It looks like the hasMedia() method isn't documented at all currently (I found it through autocomplete in my IDE), so I assume I'd just need to add this to the Retrieving Media docs?

While I'm at it, public function getFirstMedia(string $collectionName = 'default', $filters = []): ?Media isn't typehinted at all, but it calls getMedia() anyway. So for consistency I will update it to:

public function getFirstMedia(string $collectionName = 'default', array|callable $filters = []): ?Media

I don't think that change will require additional test coverage, getFirstMedia() already has tests for filters (array and callable). The only difference a user may experience is the TypeError they get by passing an invalid $filters type would come from getFirstMedia() instead of getMedia(). Same result, just one level higher.

As far as I can see those are the only methods dependent on getMedia() so we shouldn't need to change any others.