Opened 8 years ago

Last modified 7 years ago

#27317 new New feature

Make Form subclasses combine Form.Media from all parents

Reported by: James Pic Owned by: nobody
Component: Forms Version: 1.10
Severity: Normal Keywords:
Cc: James Pic Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I wanted to define a mixin to use with Form classes that would add extra JS. So, I tried how Form.Media would behave with inheritance, and it turns out they don't merge. For example:

class A(Form):
    class Media:
        js = ('jquery.js', 'a.js')

class B(Form):
    class Media:
        js = ('jquery.js', 'b.js')

class C(A, B):
    pass

I expected:

C.Media.js == ('jquery.js', 'a.js', 'b.js')

But that results instead in:

C.Media.js = ('jquery.js', 'a.js')

Eventually, I wanted to push it a bit further and use a Mixin, having exactly the same but with A(object) instead of A(Form).

Can I work on a patch for this ?

Thanks for your support

Change History (7)

comment:1 by James Pic, 8 years ago

Type: BugNew feature

comment:2 by Tim Graham, 8 years ago

Summary: Form.Media inheritanceMake Form subclasses combine Form.Media from all parents

I'm not sure if that's a good idea. As the documentation says about Meta, "Normal Python name resolution rules apply. If you have multiple base classes that declare a Meta inner class, only the first one will be used. This means the child’s Meta, if it exists, otherwise the Meta of the first parent, etc." There was a slightly related thread on django-developers were it was discussed that model Meta inheritance should be explicit rather than automatic. Can you think of any precedents in Django where automatic merging similar to what you proposed takes place?

By the way, we're thinking about ways to replace Media with a better solution, although no solution seems imminent (#22298).

comment:3 by James Pic, 8 years ago

​As the documentation says about Meta, "Normal Python name resolution rules apply. If you have multiple base classes that declare a Meta inner class, only the first one will be used.

Oh, I was talking about Media, not Meta... I don't see any case where you'd want to render a widget or form without a certain script that it requires, users would just end with broken forms or is there any other result possible from this ?

Can you think of any precedents in Django where automatic merging similar to what you proposed takes place?

I'm thinking that if Form.Media inherits from the Widget Media because they require it to function properly, not because it would be nice to have. But I might be missing something again like the explicit inheritance design decision in my initial report.

Even if the paralogism between Form.Meta and Form.Media is valid (which I recon I didn't check), explicit inheritance doesn't fix it:

In [3]: from django.forms.forms import Media

In [4]: class MediaA(Media):
   ...:     js = ('jquery.js', 'a.js')
   ...:     

In [5]: 

In [5]: class MediaB(Media):
   ...:     js = ('jquery.js', 'b.js')
   ...:     

In [6]: class MediaC(MediaA, MediaB):
   ...:     pass
   ...: 

In [7]: MediaC.js
Out[7]: ('jquery.js', 'a.js')

Shouldn't we be expecting MediaC.js = ('jquery.js', 'a.js', 'b.js') here ? I can't think of any case where we wouldn't: this multiple inheritance is after all, explicit.

By the way, we're thinking about ways to replace Media with a better solution, although no solution seems imminent (#22298).

There's no hurry, I got a solution that works until them: using a Widget to render additional script without breaking the user Form class yaayyyy :D it will be easy to change when we have another solution.

Meanwhile, should we fix inheritance in Media.js and Media.css attributes ?

Version 7, edited 8 years ago by James Pic (previous) (next) (diff)

comment:4 by Tim Graham, 8 years ago

Triage Stage: UnreviewedAccepted

I don't know, I guess we could see what the code looks like. I'm just wary of investing time in something that's discouraged and/or headed toward deprecation. Of course, I guess there are people love it.

comment:5 by James Pic, 7 years ago

Yes, because currently we can not know if we should or not add a media. For example this would allow it:

class YourForm(forms.Form):
    def add_media(self, media):
        if not media.js.find('jquery.js'):  # might have been added from another app?
            media.js.add('/yourapp/jquery.js')
        media.js.add('b.js')

Also, probably we should be limited to one Media object per request.

comment:6 by James Pic, 7 years ago

Absolutely agree with Tim, which is why i didn't invest time on this, but in adapters instead.

Full comment on the corresponding PR: https://github.com/django/django/pull/9743

comment:7 by Johannes Maron, 7 years ago

Honestly, I would prefer the "normal" inheritance, where attributes overwrite one another. I am not a big fan of the magic Django does in Model.Meta. Its drives complexity and is inconsistent too. It makes migrations and the state builder a pain... anyways. Forms. I do want to repeat the same mistakes here.

Python overwrites attributes by default and I for one would want to keep it that way.

Note: See TracTickets for help on using tickets.
Back to Top