wagtailmedia icon indicating copy to clipboard operation
wagtailmedia copied to clipboard

Breaks up AbstractMedia

Open rgs258 opened this issue 11 months ago • 3 comments

Breaks up AbstractMedia into AbstractMediaFieldAdditions and AbstractMediaBase so that wagtailmedia may be used with Wagtail's Document model as a storage and serve backend.

Addresses #236

rgs258 avatar Feb 29 '24 21:02 rgs258

@rgs258 thank you for this

[..] wagtailmedia may be used with Wagtail's Document model as a storage and serve backend

Maybe I am misunderstanding this, but I am not entirely sure this is the right way. If anything, we should provide a similar mechanism, not use the document one. Ideally Wagtail core would have a generic media serve mechanism that is then used by documents, and other packages, such as wagtailmedia use it too.

Could you provide a bit more context?

zerolab avatar Mar 01 '24 09:03 zerolab

Hey @zerolab , my approach here was to consider that media could conceptually be a kind of document. Given that, I just made it possible to extend AbstractDocument in the creation of concrete Media models. This way, all my Media classes will inherit all the features and conveniences of Documents, and get updates from Wagtail core as that is also improved. One thing missing from this commit is an example added to the readme of how to do this - I'll write that up shortly.

I accept that this created a dependency in my implementations of Media, but it also leaves any other users of this module free to ignore my pattern and continue using things as they always have.

I also agree with your ideal scenario for both how Wagtail's documents' serve functionality could/should be refactored, and how this module should take advantage of that refactoring. However, since I don't have time to see that through, I didn't proceed and instead found my inheritance solution.

rgs258 avatar Mar 01 '24 14:03 rgs258

@rgs258 thank you for the follow up. I'd be curious to see your example. In the meantime, I suppose there is nothing to prevent us from making this change as it doesn't break anything for existing implementations

zerolab avatar Mar 01 '24 15:03 zerolab