[Django] #28866: merge method of class Media doesn't work properly
Django
-------------------------------------------+------------------------------
Reporter: clincher | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: 2.0
Severity: Release blocker | Keywords: forms, media
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------+------------------------------
I was trying to use django-map-widgets with django2.0rc1 and it didn't
show the widget. The problem is that widget js-files were included before
django-admin jquery's files which it relies on.
I went deeper and found that Media.merge method are pretty much implicit
which breaks Python zen guidelines. The implicity caused by
MediaOrderConflictWarning check, without that check code might be way more
simplier. More than that, the realization of that check is not working. I
got few ideas how to solve it, but honestly I would prefer just remove it
and make the method really simple.
Example of lists with which the method doesn't work properly:
list_1 = ['admin/js/vendor/jquery/jquery.js', 'admin/js/jquery.init.js',
'admin/js/core.js', 'admin/js/admin/RelatedObjectLookups.js',
'admin/js/actions.js', 'admin/js/urlify.js', 'admin/js/prepopulate.js',
'admin/js/vendor/xregexp/xregexp.js']
list_2 = ['https://maps.googleapis.com/maps/api/js?libraries=places&key=',
'mapwidgets/js/jquery_class.js', 'mapwidgets/js/django_mw_base.js',
'mapwidgets/js/mw_google_point_field.js',
'admin/js/vendor/jquery/jquery.js', 'admin/js/jquery.init.js',
'admin/js/inlines.js']
I can easily make a patch without the warning check. Also can check the
order, but it's gonna be very different way from what's there now.
I set serverity to blocker since it's totally breaks possibility to use
Django with many other libraries
--
Ticket URL: <https://code.djangoproject.com/ticket/28866>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: clincher | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 2.0
Severity: Release blocker | Resolution:
Keywords: forms, media | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* cc: Johannes Hoppe (added)
* type: Uncategorized => Bug
Comment:
How do you propose to solve the problem described in #28377 if the merging
logic is removed?
Is it impossible to modifying the ordering of your lists so the warning
doesn't happen? This is documented as a backwards incompatible change in
the [https://docs.djangoproject.com/en/dev/releases/2.0/#miscellaneous
Django 2.0 release notes]:
> In older versions, forms and formsets combine their `Media` with widget
`Media` by concatenating the two. The combining now tries to
[https://docs.djangoproject.com/en/dev/topics/forms/media/#form-media-
asset-order preserve the relative order of elements in each list].
`MediaOrderConflictWarning` is issued if the order can’t be preserved.
--
Ticket URL: <https://code.djangoproject.com/ticket/28866#comment:1>
Django
Reporter: clincher | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 2.0
Severity: Release blocker | Resolution:
Keywords: forms, media | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Johannes Hoppe):
Hello there!
I was the guy who wrote the implicit and none zen conform code ;) Do you
happen to have a trace that warning or an example that I could reproduce
it myself?
That aside, Tim is correct, the changes are required in #28377. I would be
curious myself how to improve it. I currently don't see how not throwing a
warning would improve verbosity of the code and its behavior. Could you
elaborate more on how you patch would would work?
I the mean time I will have a look at the package you talked about to get
a better understanding of the problem.
Cheers
-Johannes
--
Ticket URL: <https://code.djangoproject.com/ticket/28866#comment:2>
Django
Reporter: clincher | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 2.0
Severity: Release blocker | Resolution:
Keywords: forms, media | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by clincher):
Hi guys, thanks for fast response!
I know that static assets must not be duplicated as well as follow
predefined order. The easiest way to do it will be like that:
for item in list_2:
if item not in list_1:
list_1.append(item)
it will follow all the orders and not duplicate anything.
Personally I find it overcaring to throw the warning about unmatching
order in different lists (Media). However if you are sure it is super
necessary, than I don't find the current logic proper. I barely experience
problem with reading other's people code, but the current method blowed
me. However I think the current algorithm is not correct (as it shown with
data I provided in the ticket) and probably it's not possible way to solve
it.
For throwing the warning (if you really badly need it, guys), we can do
that way:
order_list = []
for item in list_2:
try:
order_list.append(list_1.index(item))
except ValueError:
pass
if order_list is not sort(order_list):
OrderWarning(...)
So, the idea will be that we can associate order of first and second list
and if it is not strictly incremental then throw the warning.
However, seriously, guys, I find it too much and still not perfect,
because there can be a cases when in list_2 will be additional paths
before/after and we can't take care of all the files in third-party
libraries. But what we must do is make Django work as expected: merge
lists of paths in the proper way, not as it is now. In other words better
we will have working explicit solution without warning rather than buggy
solution with warning. What do you think?
--
Ticket URL: <https://code.djangoproject.com/ticket/28866#comment:3>
Django
Reporter: clincher | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 2.0
Severity: Release blocker | Resolution:
Keywords: forms, media | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Have you read [https://docs.djangoproject.com/en/dev/topics/forms/media
/#form-media-asset-order the documentation about how merging is supposed
to work]?
If you want jQuery included before the Google Maps stuff, then you need to
reorder `list_2` so that jQuery is before it. Something like
`['admin/js/vendor/jquery/jquery.js', 'admin/js/jquery.init.js',
'admin/js/inlines.js',
'https://maps.googleapis.com/maps/api/js?libraries=places&key=',
'mapwidgets/js/jquery_class.js', 'mapwidgets/js/django_mw_base.js',
'mapwidgets/js/mw_google_point_field.js']`.
Is there a reason you can't do that?
In older versions of Django, the resulting Media only happened to work
because jQuery was used in `list_1`. I think the new ordering logic that
tries to preserve relative ordering in the two media lists is more
intuitive.
--
Ticket URL: <https://code.djangoproject.com/ticket/28866#comment:4>
Django
Reporter: clincher | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 2.0
Severity: Release blocker | Resolution:
Keywords: forms, media | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Johannes Hoppe):
Hi @clincher,
let me try to go though you points one by one:
This
{{{
for item in list_2:
if item not in list_1:
list_1.append(item)
}}}
does not work. It avoids duplicates but does not preserve the order. This
was the whole point of #28377
This I don't really understand, but it does not preserve the order ether.
You will have to go though it reversed.
{{{
order_list = []
for item in list_2:
try:
order_list.append(list_1.index(item))
except ValueError:
pass
if order_list is not sort(order_list):
OrderWarning(...)
}}}
Your example
{{{
list_1= [path1, path2, path3]
list_2 = [path4, path2, path5]
}}}
does work, as such:
{{{
>>> list_1= ['path1', 'path2', 'path3']
>>> list_2 = ['path4', 'path2', 'path5']
>>> from django.forms.widgets import Media
>>> Media.merge(list_1, list_2)
['path1', 'path4', 'path2', 'path3', 'path5']
}}}
It does preserve the order of both initial lists.
I know this is a difficult topic and the algorithm isn't easy to
understand, but to my knowledge it's the only way to achieve both
uniqueness and order.
The warning is just so that you are alerted that your widget setup might
now work, which it doesn't in your case. I would consider this a good
thing, because it alerted you to the issue of sorting.
--
Ticket URL: <https://code.djangoproject.com/ticket/28866#comment:5>
Django
Reporter: clincher | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 2.0
Severity: Release blocker | Resolution:
Keywords: forms, media | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by clincher):
Thanks for the pointing to my fault, @Tim
Thanks a lot for explanaition, @ Johannes
Then everything works fine in the merge method and the actual issue is not
in the forms app, but django.admin.helpers in the method media of
InlineAdminFormSet, line 307 shall be just changed from to
{{{
media = self.formset.media + self.opts.media
}}}
to
{{{
media = self.opts.media + self.formset.media
}}}
Because this is a place where it put opts.media (jquery.js,
jquery.init.js, inlines.js) before the formset.media where it actually
utilized.
Shall I create a patch or?
Cheers, Vasiliy
--
Ticket URL: <https://code.djangoproject.com/ticket/28866#comment:6>
Django
Reporter: clincher | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 2.0
Severity: Release blocker | Resolution:
Keywords: forms, media | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Johannes Hoppe):
Hi Vasiliy,
sure please create a patch. I would be happy to review it.
A little side note, I remember that the admin already has some form media
defined that includes jQuery. I don't believe that is needed anymore.
Actually jQuery should only be imported if actually used by a widget. So
the only place the jQuery media should be included is there.
This duplication could be the reason for your troubles or make your patch
more difficult. Fingers crossed! Anyways, I am here to help should you
need me :)
-Joe
--
Ticket URL: <https://code.djangoproject.com/ticket/28866#comment:7>
Django
Reporter: clincher | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 2.0
Severity: Release blocker | Resolution:
Keywords: formset, media, | Triage Stage:
admin | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by clincher):
* keywords: forms, media => formset, media, admin
* has_patch: 0 => 1
* component: Forms => contrib.admin
* easy: 0 => 1
Comment:
I made the pull request https://github.com/django/django/pull/9404
Not sure if I have to write about it here
Cheers, Vasiliy
--
Ticket URL: <https://code.djangoproject.com/ticket/28866#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: clincher | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 2.0
Severity: Release blocker | Resolution:
Keywords: formset, media, | Triage Stage:
admin | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/28866#comment:9>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 2.0
Severity: Release blocker | Resolution:
admin |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_tests: 0 => 1
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/28866#comment:10>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 2.0
Severity: Release blocker | Resolution:
Keywords: formset, media, | Triage Stage: Accepted
admin |
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
Did you test the fix? In my testing it does not resolve the issue (as I
would expect). The order in which the two lists are combined should make
no difference. The relative ordering of the elements in the two lists is
preserved. I believe django-map-widgets must fix this issue as I outlined
in comment:4. I'll take a closer look and perhaps propose a fix there.
--
Ticket URL: <https://code.djangoproject.com/ticket/28866#comment:11>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 2.0
Severity: Release blocker | Resolution:
Keywords: formset, media, | Triage Stage: Accepted
admin |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
I take that last comment back. I made a mistake in testing and I see that
it's working now. I'll work on a test.
--
Ticket URL: <https://code.djangoproject.com/ticket/28866#comment:12>
Django
formset's Media
-------------------------------------+-------------------------------------
Reporter: clincher | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 2.0
Severity: Release blocker | Resolution:
Keywords: formset, media, | Triage Stage: Accepted
admin |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_tests: 1 => 0
* easy: 1 => 0
Comment:
I added a test to the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/28866#comment:13>
Django
formset's Media
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 2.0
Severity: Release blocker | Resolution:
admin | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/28866#comment:14>
Django
formset's Media
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: contrib.admin | Version: 2.0
Severity: Release blocker | Resolution: fixed
Keywords: formset, media, | Triage Stage: Ready for
admin | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"03974d81220ffd237754a82c77913799dd5909a4" 03974d81]:
{{{
#!CommitTicketReference repository=""
revision="03974d81220ffd237754a82c77913799dd5909a4"
Fixed #28866 -- Made InlineAdminFormSet include InlineModelAdmin's Media
before its formset's Media.
This provides better backwards compatibility following refs #28377.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28866#comment:15>
Django
formset's Media
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: contrib.admin | Version: 2.0
Severity: Release blocker | Resolution: fixed
Keywords: formset, media, | Triage Stage: Ready for
admin | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"6ece69a7264bf45f6ddac5afafeb7de6c13df64f" 6ece69a7]:
{{{
#!CommitTicketReference repository=""
revision="6ece69a7264bf45f6ddac5afafeb7de6c13df64f"
[2.0.x] Fixed #28866 -- Made InlineAdminFormSet include InlineModelAdmin's
Media before its formset's Media.
This provides better backwards compatibility following refs #28377.
Backport of 03974d81220ffd237754a82c77913799dd5909a4 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28866#comment:16>