Skip to content

Breaks up AbstractMedia - #238

Open
rgs258 wants to merge 1 commit into
torchbox:mainfrom
rgs258:Break-Up-AbstractMedia
Open

Breaks up AbstractMedia#238
rgs258 wants to merge 1 commit into
torchbox:mainfrom
rgs258:Break-Up-AbstractMedia

Conversation

@rgs258

@rgs258 rgs258 commented Feb 29, 2024

Copy link
Copy Markdown

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

…MediaBase so that WagtailMedia may be used with Wagtail's Document model as a storage and serve backend.
@zerolab

zerolab commented Mar 1, 2024

Copy link
Copy Markdown
Member

@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?

@rgs258

rgs258 commented Mar 1, 2024

Copy link
Copy Markdown
Author

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.

@zerolab

zerolab commented Mar 1, 2024

Copy link
Copy Markdown
Member

@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

verbose_name = _("media")


class AbstractMediaBase(index.Indexed, models.Model):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm.. this is not quite there as AbstractMediaBase defines search_fields and admin_form_fields that depend on fields that are now on AbstractMediaFieldAdditions

@zerolab
zerolab force-pushed the main branch 4 times, most recently from ddccf3a to b39f564 Compare October 18, 2025 11:23
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.

2 participants