#30153 closed Bug (fixed)
ModelAdmin with custom widgets, inlines, and filter_horizontal can merge media in broken order
| Reported by: | roybi | Owned by: | nobody |
|---|---|---|---|
| Component: | contrib.admin | Version: | 2.1 |
| Severity: | Normal | Keywords: | |
| Cc: | clincher, Johannes Maron, Matthias Kestenholz | Triage Stage: | Accepted |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
when a modeadmin have a inline with a filed has its own media js no need jquery , and have a one to many field show filter_horizontal, the problem appear.
there will be MediaOrderConflictWarning and inlines.js load before jquery.
Attachments (3)
Change History (16)
by , 6 years ago
| Attachment: | inlinetest.tgz added |
|---|
comment:1 by , 6 years ago
| Summary: | inlines.js merge with wrong order in modeladmin → Admin: inlines.js merge with wrong order in modeladmin |
|---|
comment:2 by , 6 years ago
| Cc: | added |
|---|---|
| Summary: | Admin: inlines.js merge with wrong order in modeladmin → ModelAdmin with custom widgets, inlines, and filter_horizontal can merge media in broken order |
| Triage Stage: | Unreviewed → Accepted |
I dove into this for a while. I'm not sure if there's anything that Django can do about it. Reverting 03974d81220ffd237754a82c77913799dd5909a4 solves the problem but that'll break other cases.
The root causes seems to be that ckeditor/ckeditor/ckeditor.js is collected before jquery in one case and after it in another.
comment:3 by , 6 years ago
| Cc: | added |
|---|
comment:4 by , 6 years ago
It seems to me that Django assumes too much. Here are some assets that have been collected in my case:
[
'imagefield/ppoi.js',
<JS(ckeditor/ckeditor-init.js, {'id': 'ckeditor-init-script', 'data-ckeditor-basepath': '/static/ckeditor/ckeditor/'})>,
'ckeditor/ckeditor/ckeditor.js',
'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',
<JS(https://use.fontawesome.com/releases/v5.3.1/js/all.js, {'async': 'async', 'integrity': 'sha384-kW+oWsYx3YpxvjtZjFXqazFpA7UP/MbiY4jvs+RWZo2+N94PFZ36T6TFkc9O3qoB', 'crossorigin': 'anonymous'})>,
'app/plugin_buttons.js',
'admin/js/calendar.js',
'admin/js/admin/DateTimeShortcuts.js'
]
The imagefield and ckeditor assets are there because of widgets. When using the same widgets in inlines the inlines' Media class will contain jquery, jquery.init.js, inlines.js, imagefield/ppoi.js. When merging the two JS lists Django will find that imagefield/ppoi.js is at index 0, will continue with inlines.js (and of course not find it) and insert it at index 0 as well (because that's the value of last_insert_index now). As soon as jquery.init.js is encountered it notices that something is amiss and emits a MediaOrderConflictWarning. The problem was produced one iteration earlier and the error message is not very helpful.
I don't have a good suggestion yet. It also baffles me that only one of two candidate models/modeladmins shows the problem, and (for now) luckily only in development.
comment:5 by , 6 years ago
Following up. This problem exists since Django 2.0. It doesn't show itself in Django 1.11.x (not surprising since the MediaOrderConflictWarning change was introduced with Django 2.0). It's good that I'm able to reproduce this on a different computer, so that gives me hope that it's not impossible to debug & fix.
It's unfortunate that the media files which cause the reordering don't even depend on jQuery.
comment:6 by , 6 years ago
Hi there, I was the one introducing the warning. The warning is emitted when the defined asset order can not be maintained. Which is a good thing to warn about, but not always a problem. It is particularly not an issue, if you have nested forms, where assets from multiple fields are merged into preliminary forms (like inlines) just to be merged again.
I think a real solution could be to retain information about the explicitly defined order-constraints and ignore the implicit once. The merging algorithm can stay as is, it is correct. All that would need to be done, is order violation violates an implicit or explicit asset order before emitting the warning.
This would however use a bit more memory, since we would need to keep a record of constraints (list of tuples).
What do you think? I could invest a bit of time, to draft a solution.
comment:7 by , 6 years ago
Hey, yes that might be the right thing to do but maybe there is a solution which requires less effort. I suspect that the problem is an inconsistency in the way Django collects media from different places. I inserted a few print() statements here https://github.com/django/django/blob/893b80d95dd76642e478893ba6d4f46bb31388f1/django/contrib/admin/options.py#L1595
(Sorry for the big blob in advance)
self.media <script type="text/javascript" src="/static/admin/js/vendor/jquery/jquery.js"></script> <script type="text/javascript" src="/static/admin/js/jquery.init.js"></script> <script type="text/javascript" src="/static/admin/js/core.js"></script> <script type="text/javascript" src="/static/admin/js/admin/RelatedObjectLookups.js"></script> <script type="text/javascript" src="/static/admin/js/actions.js"></script> <script type="text/javascript" src="/static/admin/js/urlify.js"></script> <script type="text/javascript" src="/static/admin/js/prepopulate.js"></script> <script type="text/javascript" src="/static/admin/js/vendor/xregexp/xregexp.js"></script> <script type="text/javascript" src="https://use.fontawesome.com/releases/v5.3.1/js/all.js" async="async" crossorigin="anonymous" integrity="sha384-kW+oWsYx3YpxvjtZjFXqazFpA7UP/MbiY4jvs+RWZo2+N94PFZ36T6TFkc9O3qoB"></script> <script type="text/javascript" src="/static/app/plugin_buttons.js"></script> adminForm.media <link href="/static/imagefield/ppoi.css" type="text/css" media="screen" rel="stylesheet"> <script type="text/javascript" src="/static/imagefield/ppoi.js"></script> <script type="text/javascript" src="/static/ckeditor/ckeditor-init.js" data-ckeditor-basepath="/static/ckeditor/ckeditor/" id="ckeditor-init-script"></script> <script type="text/javascript" src="/static/ckeditor/ckeditor/ckeditor.js"></script> <script type="text/javascript" src="/static/admin/js/vendor/jquery/jquery.js"></script> <script type="text/javascript" src="/static/admin/js/jquery.init.js"></script> <script type="text/javascript" src="/static/admin/js/calendar.js"></script> <script type="text/javascript" src="/static/admin/js/admin/DateTimeShortcuts.js"></script> inline_formset.media <class 'app.articles.models.RichText'> <script type="text/javascript" src="/static/admin/js/vendor/jquery/jquery.js"></script> <script type="text/javascript" src="/static/admin/js/jquery.init.js"></script> <script type="text/javascript" src="/static/admin/js/inlines.js"></script> <script type="text/javascript" src="/static/feincms3/plugin_ckeditor.js"></script> <script type="text/javascript" src="/static/ckeditor/ckeditor-init.js" data-ckeditor-basepath="/static/ckeditor/ckeditor/" id="ckeditor-init-script"></script> <script type="text/javascript" src="/static/ckeditor/ckeditor/ckeditor.js"></script> inline_formset.media <class 'app.articles.models.Image'> <link href="/static/imagefield/ppoi.css" type="text/css" media="screen" rel="stylesheet"> <script type="text/javascript" src="/static/admin/js/vendor/jquery/jquery.js"></script> <script type="text/javascript" src="/static/admin/js/jquery.init.js"></script> <script type="text/javascript" src="/static/admin/js/inlines.js"></script> <script type="text/javascript" src="/static/imagefield/ppoi.js"></script>
So somehow the widget media files (imagefield and ckeditor) come after the files added by Django's inlines in inline formsets but before them in the helpers.AdminForm belonging to the ArticleAdmin class.
The problem manifests itself when having any third-party widget (which does not reference Django's jquery asset) before any Django date field, prepopulated field or filter_* field in the fieldsets structure. Reordering fieldsets (or fields) avoids the problem completely. Now I still don't understand why the exact same project would work on the server and not locally.
The problem can be worked around by including "admin/js/vendor/jquery/jquery%s.js", "admin/js/jquery.init.js" in third-party widgets' Media definitions. This sucks big time though, especially since those widgets don't even require jQuery to work.
jQuery is included on all modeladmin pages anyway, so a good fix might be to remove the jquery and jquery.init.js entries from admin widgets Media definitions (which makes them incomplete when used outside of Django's admin panel, but that's probably not the intention anyway) or make the Media merging algorithm aware of libraries which are supposed to always come first.
Just for reference, here's the form class and its fields. Putting e.g. publication_date before image makes everything work fine.
form.media <link href="/static/imagefield/ppoi.css" type="text/css" media="screen" rel="stylesheet"> <script type="text/javascript" src="/static/imagefield/ppoi.js"></script> <script type="text/javascript" src="/static/ckeditor/ckeditor-init.js" data-ckeditor-basepath="/static/ckeditor/ckeditor/" id="ckeditor-init-script"></script> <script type="text/javascript" src="/static/ckeditor/ckeditor/ckeditor.js"></script> <script type="text/javascript" src="/static/admin/js/vendor/jquery/jquery.js"></script> <script type="text/javascript" src="/static/admin/js/jquery.init.js"></script> <script type="text/javascript" src="/static/admin/js/calendar.js"></script> <script type="text/javascript" src="/static/admin/js/admin/DateTimeShortcuts.js"></script> form.fields is_active: <class 'django.forms.fields.BooleanField'> title: <class 'django.forms.fields.CharField'> excerpt: <class 'django.forms.fields.CharField'> image: <class 'django.forms.fields.ImageField'> image_ppoi: <class 'django.forms.fields.CharField'> meta_title: <class 'django.forms.fields.CharField'> meta_description: <class 'django.forms.fields.CharField'> meta_image: <class 'django.forms.fields.ImageField'> meta_canonical: <class 'django.forms.fields.URLField'> meta_author: <class 'django.forms.fields.CharField'> meta_robots: <class 'django.forms.fields.CharField'> show_teaser_need: <class 'django.forms.fields.BooleanField'> show_teaser_competency: <class 'django.forms.fields.BooleanField'> teaser_title: <class 'django.forms.fields.CharField'> teaser_text_need: <class 'ckeditor.fields.RichTextFormField'> teaser_text_competency: <class 'ckeditor.fields.RichTextFormField'> teaser_image: <class 'django.forms.fields.ImageField'> teaser_image_ppoi: <class 'django.forms.fields.CharField'> slug: <class 'django.forms.fields.SlugField'> publication_date: <class 'django.forms.fields.SplitDateTimeField'> is_featured: <class 'django.forms.fields.BooleanField'> author: <class 'django.forms.models.ModelChoiceField'> categories: <class 'django.forms.models.ModelMultipleChoiceField'>
comment:8 by , 6 years ago
I just attached a minimal patch demonstrating the problem: https://code.djangoproject.com/attachment/ticket/30153/test.patch
Simply reordering date and dummy in Holder3 makes the problem go away in this case (since Holder3's modeladmin class does not define fieldsets)
by , 6 years ago
| Attachment: | test.2.patch added |
|---|
comment:9 by , 6 years ago
Replacing the patch with https://code.djangoproject.com/attachment/ticket/30153/test.2.patch because Django 2.2 and 3 will not use jquery in date fields anymore and therefore cannot be used to demonstrate the problem. I changed the code to use a filter_horizontal widget and now it fails again:
======================================================================
ERROR: test_inline_media_only_inline (admin_inlines.tests.TestInlineMedia)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib/python3.6/unittest/case.py", line 59, in testPartExecutor
yield
File "/usr/lib/python3.6/unittest/case.py", line 605, in run
testMethod()
File "/home/matthias/Projects/django/tests/admin_inlines/tests.py", line 495, in test_inline_media_only_inline
response = self.client.get(change_url)
File "/home/matthias/Projects/django/django/test/client.py", line 535, in get
response = super().get(path, data=data, secure=secure, **extra)
File "/home/matthias/Projects/django/django/test/client.py", line 347, in get
**extra,
File "/home/matthias/Projects/django/django/test/client.py", line 422, in generic
return self.request(**r)
File "/home/matthias/Projects/django/django/test/client.py", line 503, in request
raise exc_value
File "/home/matthias/Projects/django/django/core/handlers/exception.py", line 34, in inner
response = get_response(request)
File "/home/matthias/Projects/django/django/core/handlers/base.py", line 115, in _get_response
response = self.process_exception_by_middleware(e, request)
File "/home/matthias/Projects/django/django/core/handlers/base.py", line 113, in _get_response
response = wrapped_callback(request, *callback_args, **callback_kwargs)
File "/home/matthias/Projects/django/django/contrib/admin/options.py", line 604, in wrapper
return self.admin_site.admin_view(view)(*args, **kwargs)
File "/home/matthias/Projects/django/django/utils/decorators.py", line 142, in _wrapped_view
response = view_func(request, *args, **kwargs)
File "/home/matthias/Projects/django/django/views/decorators/cache.py", line 44, in _wrapped_view_func
response = view_func(request, *args, **kwargs)
File "/home/matthias/Projects/django/django/contrib/admin/sites.py", line 223, in inner
return view(request, *args, **kwargs)
File "/home/matthias/Projects/django/django/contrib/admin/options.py", line 1635, in change_view
return self.changeform_view(request, object_id, form_url, extra_context)
File "/home/matthias/Projects/django/django/utils/decorators.py", line 45, in _wrapper
return bound_method(*args, **kwargs)
File "/home/matthias/Projects/django/django/utils/decorators.py", line 142, in _wrapped_view
response = view_func(request, *args, **kwargs)
File "/home/matthias/Projects/django/django/contrib/admin/options.py", line 1520, in changeform_view
return self._changeform_view(request, object_id, form_url, extra_context)
File "/home/matthias/Projects/django/django/contrib/admin/options.py", line 1594, in _changeform_view
media = media + inline_formset.media
File "/home/matthias/Projects/django/django/forms/widgets.py", line 135, in __add__
combined._js = self.merge(self._js, other._js)
File "/home/matthias/Projects/django/django/forms/widgets.py", line 126, in merge
MediaOrderConflictWarning,
django.forms.widgets.MediaOrderConflictWarning: Detected duplicate Media files in an opposite order:
admin/js/inlines.min.js
admin/js/jquery.init.js
comment:10 by , 6 years ago
@codingjoe Here's a partial fix for the issue -- https://github.com/matthiask/django/commit/0640ba9f5f6272987b77c35d5ad992844d6a8822
Preserving the JavaScript lists longer and only merging them at the end works. Or maybe you have a better idea? Feel free to take over if you do (or even if you don't -- if you want to)
This is the test program