Fixed #28377 -- Made combining form Media retain relative asset order.

Thanks Florian Apolloner, Mariusz Felisiak, and Tim Graham for reviews.

django/contrib/admin/helpers.py

@@ -304,7 +304,7 @@ def non_form_errors(self):
 
     @property
     def media(self):
-        media = self.opts.media + self.formset.media
+        media = self.formset.media + self.opts.media
         for fs in self:
             media = media + fs.media
         return media

django/contrib/admin/options.py

@@ -579,9 +579,9 @@ def urls(self):
     def media(self):
         extra = '' if settings.DEBUG else '.min'
         js = [
-            'core.js',
             'vendor/jquery/jquery%s.js' % extra,
             'jquery.init.js',
+            'core.js',
             'admin/RelatedObjectLookups.js',
             'actions%s.js' % extra,
             'urlify.js',

django/contrib/admin/widgets.py

@@ -4,6 +4,7 @@
 import copy
 
 from django import forms
+from django.conf import settings
 from django.db.models.deletion import CASCADE
 from django.urls import reverse
 from django.urls.exceptions import NoReverseMatch
@@ -22,7 +23,14 @@ class FilteredSelectMultiple(forms.SelectMultiple):
     """
     @property
     def media(self):
-        js = ["core.js", "SelectBox.js", "SelectFilter2.js"]
+        extra = '' if settings.DEBUG else '.min'
+        js = [
+            'vendor/jquery/jquery%s.js' % extra,
+            'jquery.init.js',
+            'core.js',
+            'SelectBox.js',
+            'SelectFilter2.js',
+        ]
         return forms.Media(js=["admin/js/%s" % path for path in js])
 
     def __init__(self, verbose_name, is_stacked, attrs=None, choices=()):
@@ -43,7 +51,13 @@ def get_context(self, name, value, attrs):
 class AdminDateWidget(forms.DateInput):
     @property
     def media(self):
-        js = ["calendar.js", "admin/DateTimeShortcuts.js"]
+        extra = '' if settings.DEBUG else '.min'
+        js = [
+            'vendor/jquery/jquery%s.js' % extra,
+            'jquery.init.js',
+            'calendar.js',
+            'admin/DateTimeShortcuts.js',
+        ]
         return forms.Media(js=["admin/js/%s" % path for path in js])
 
     def __init__(self, attrs=None, format=None):
@@ -56,7 +70,13 @@ def __init__(self, attrs=None, format=None):
 class AdminTimeWidget(forms.TimeInput):
     @property
     def media(self):
-        js = ["calendar.js", "admin/DateTimeShortcuts.js"]
+        extra = '' if settings.DEBUG else '.min'
+        js = [
+            'vendor/jquery/jquery%s.js' % extra,
+            'jquery.init.js',
+            'calendar.js',
+            'admin/DateTimeShortcuts.js',
+        ]
         return forms.Media(js=["admin/js/%s" % path for path in js])
 
     def __init__(self, attrs=None, format=None):

django/contrib/gis/admin/options.py

@@ -2,6 +2,7 @@
 from django.contrib.gis.admin.widgets import OpenLayersWidget
 from django.contrib.gis.db import models
 from django.contrib.gis.gdal import OGRGeomType
+from django.forms import Media
 
 spherical_mercator_srid = 3857
 
@@ -46,10 +47,7 @@ class GeoModelAdmin(ModelAdmin):
     @property
     def media(self):
         "Injects OpenLayers JavaScript into the admin."
-        media = super().media
-        media.add_js([self.openlayers_url])
-        media.add_js(self.extra_js)
-        return media
+        return super().media + Media(js=[self.openlayers_url] + self.extra_js)
 
     def formfield_for_dbfield(self, db_field, request, **kwargs):
         """

django/forms/widgets.py

@@ -5,6 +5,7 @@
 import copy
 import datetime
 import re
+import warnings
 from contextlib import suppress
 from itertools import chain
 
@@ -33,19 +34,23 @@
 MEDIA_TYPES = ('css', 'js')
 
 
+class MediaOrderConflictWarning(RuntimeWarning):
+    pass
+
+
 @html_safe
 class Media:
-    def __init__(self, media=None, **kwargs):
-        if media:
-            media_attrs = media.__dict__
+    def __init__(self, media=None, css=None, js=None):
+        if media is not None:
+            css = getattr(media, 'css', {})
+            js = getattr(media, 'js', [])
         else:
-            media_attrs = kwargs
-
-        self._css = {}
-        self._js = []
-
-        for name in MEDIA_TYPES:
-            getattr(self, 'add_' + name)(media_attrs.get(name))
+            if css is None:
+                css = {}
+            if js is None:
+                js = []
+        self._css = css
+        self._js = js
 
     def __str__(self):
         return self.render()
@@ -88,24 +93,48 @@ def __getitem__(self, name):
             return Media(**{str(name): getattr(self, '_' + name)})
         raise KeyError('Unknown media type "%s"' % name)
 
-    def add_js(self, data):
-        if data:
-            for path in data:
-                if path not in self._js:
-                    self._js.append(path)
+    @staticmethod
+    def merge(list_1, list_2):
+        """
+        Merge two lists while trying to keep the relative order of the elements.
+        Warn if the lists have the same two elements in a different relative
+        order.
 
-    def add_css(self, data):
-        if data:
-            for medium, paths in data.items():
-                for path in paths:
-                    if not self._css.get(medium) or path not in self._css[medium]:
-                        self._css.setdefault(medium, []).append(path)
+        For static assets it can be important to have them included in the DOM
+        in a certain order. In JavaScript you may not be able to reference a
+        global or in CSS you might want to override a style.
+        """
+        # Start with a copy of list_1.
+        combined_list = list(list_1)
+        last_insert_index = len(list_1)
+        # Walk list_2 in reverse, inserting each element into combined_list if
+        # it doesn't already exist.
+        for path in reversed(list_2):
+            try:
+                # Does path already exist in the list?
+                index = combined_list.index(path)
+            except ValueError:
+                # Add path to combined_list since it doesn't exist.
+                combined_list.insert(last_insert_index, path)
+            else:
+                if index > last_insert_index:
+                    warnings.warn(
+                        'Detected duplicate Media files in an opposite order:\n'
+                        '%s\n%s' % (combined_list[last_insert_index], combined_list[index]),
+                        MediaOrderConflictWarning,
+                    )
+                # path already exists in the list. Update last_insert_index so
+                # that the following elements are inserted in front of this one.
+                last_insert_index = index
+        return combined_list
 
     def __add__(self, other):
         combined = Media()
-        for name in MEDIA_TYPES:
-            getattr(combined, 'add_' + name)(getattr(self, '_' + name, None))
-            getattr(combined, 'add_' + name)(getattr(other, '_' + name, None))
+        combined._js = self.merge(self._js, other._js)
+        combined._css = {
+            medium: self.merge(self._css.get(medium, []), other._css.get(medium, []))
+            for medium in self._css.keys() | other._css.keys()
+        }
         return combined
 
 

docs/releases/2.0.txt

@@ -554,6 +554,11 @@ Miscellaneous
 * Renamed ``BaseExpression._output_field`` to ``output_field``. You may need
   to update custom expressions.
 
+* In older versions, forms and formsets combine their ``Media`` with widget
+  ``Media`` by concatenating the two. The combining now tries to :ref:`preserve
+  the relative order of elements in each list <form-media-asset-order>`.
+  ``MediaOrderConflictWarning`` is issued if the order can't be preserved.
+
 .. _deprecated-features-2.0:
 
 Features deprecated in 2.0

docs/topics/forms/media.txt

@@ -305,6 +305,42 @@ specified by both::
     <script type="text/javascript" src="http://static.example.com/actions.js"></script>
     <script type="text/javascript" src="http://static.example.com/whizbang.js"></script>
 
+.. _form-media-asset-order:
+
+Order of assets
+---------------
+
+The order in which assets are inserted into the DOM if often important. For
+example, you may have a script that depends on jQuery. Therefore, combining
+``Media`` objects attempts to preserve the relative order in which assets are
+defined in each ``Media`` class.
+
+For example::
+
+    >>> from django import forms
+    >>> class CalendarWidget(forms.TextInput):
+    ...     class Media:
+    ...         js = ('jQuery.js', 'calendar.js', 'noConflict.js')
+    >>> class TimeWidget(forms.TextInput):
+    ...     class Media:
+    ...         js = ('jQuery.js', 'time.js', 'noConflict.js')
+    >>> w1 = CalendarWidget()
+    >>> w2 = TimeWidget()
+    >>> print(w1.media + w2.media)
+    <script type="text/javascript" src="http://static.example.com/jQuery.js"></script>
+    <script type="text/javascript" src="http://static.example.com/calendar.js"></script>
+    <script type="text/javascript" src="http://static.example.com/time.js"></script>
+    <script type="text/javascript" src="http://static.example.com/noConflict.js"></script>
+
+Combining ``Media`` objects with assets in a conflicting order results in a
+``MediaOrderConflictWarning``.
+
+.. versionchanged:: 2.0
+
+    In older versions, the assets of ``Media`` objects are concatenated rather
+    than merged in a way that tries to preserve the relative ordering of the
+    elements in each list.
+
 ``Media`` on Forms
 ==================
 

tests/forms_tests/tests/test_media.py

@@ -1,3 +1,5 @@
+import warnings
+
 from django.forms import CharField, Form, Media, MultiWidget, TextInput
 from django.template import Context, Template
 from django.test import SimpleTestCase, override_settings
@@ -106,7 +108,7 @@ class Media:
         class MyWidget3(TextInput):
             class Media:
                 css = {
-                    'all': ('/path/to/css3', 'path/to/css1')
+                    'all': ('path/to/css1', '/path/to/css3')
                 }
                 js = ('/path/to/js1', '/path/to/js4')
 
@@ -237,9 +239,9 @@ class Media:
         w8 = MyWidget8()
         self.assertEqual(
             str(w8.media),
-            """<link href="http://media.example.com/static/path/to/css1" type="text/css" media="all" rel="stylesheet" />
+            """<link href="/path/to/css3" type="text/css" media="all" rel="stylesheet" />
+<link href="http://media.example.com/static/path/to/css1" type="text/css" media="all" rel="stylesheet" />
 <link href="/path/to/css2" type="text/css" media="all" rel="stylesheet" />
-<link href="/path/to/css3" type="text/css" media="all" rel="stylesheet" />
 <script type="text/javascript" src="/path/to/js1"></script>
 <script type="text/javascript" src="http://media.other.com/path/to/js2"></script>
 <script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>
@@ -312,9 +314,9 @@ class Media:
         w11 = MyWidget11()
         self.assertEqual(
             str(w11.media),
-            """<link href="http://media.example.com/static/path/to/css1" type="text/css" media="all" rel="stylesheet" />
+            """<link href="/path/to/css3" type="text/css" media="all" rel="stylesheet" />
+<link href="http://media.example.com/static/path/to/css1" type="text/css" media="all" rel="stylesheet" />
 <link href="/path/to/css2" type="text/css" media="all" rel="stylesheet" />
-<link href="/path/to/css3" type="text/css" media="all" rel="stylesheet" />
 <script type="text/javascript" src="/path/to/js1"></script>
 <script type="text/javascript" src="http://media.other.com/path/to/js2"></script>
 <script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>
@@ -341,9 +343,9 @@ class Media:
         w12 = MyWidget12()
         self.assertEqual(
             str(w12.media),
-            """<link href="http://media.example.com/static/path/to/css1" type="text/css" media="all" rel="stylesheet" />
+            """<link href="/path/to/css3" type="text/css" media="all" rel="stylesheet" />
+<link href="http://media.example.com/static/path/to/css1" type="text/css" media="all" rel="stylesheet" />
 <link href="/path/to/css2" type="text/css" media="all" rel="stylesheet" />
-<link href="/path/to/css3" type="text/css" media="all" rel="stylesheet" />
 <script type="text/javascript" src="/path/to/js1"></script>
 <script type="text/javascript" src="/path/to/js4"></script>"""
         )
@@ -396,7 +398,7 @@ class Media:
         class MyWidget3(TextInput):
             class Media:
                 css = {
-                    'all': ('/path/to/css3', 'path/to/css1')
+                    'all': ('path/to/css1', '/path/to/css3')
                 }
                 js = ('/path/to/js1', '/path/to/js4')
 
@@ -441,7 +443,7 @@ class Media:
         class MyWidget3(TextInput):
             class Media:
                 css = {
-                    'all': ('/path/to/css3', 'path/to/css1')
+                    'all': ('path/to/css1', '/path/to/css3')
                 }
                 js = ('/path/to/js1', '/path/to/js4')
 
@@ -518,3 +520,25 @@ def test_html_safe(self):
         media = Media(css={'all': ['/path/to/css']}, js=['/path/to/js'])
         self.assertTrue(hasattr(Media, '__html__'))
         self.assertEqual(str(media), media.__html__())
+
+    def test_merge(self):
+        test_values = (
+            (([1, 2], [3, 4]), [1, 2, 3, 4]),
+            (([1, 2], [2, 3]), [1, 2, 3]),
+            (([2, 3], [1, 2]), [1, 2, 3]),
+            (([1, 3], [2, 3]), [1, 2, 3]),
+            (([1, 2], [1, 3]), [1, 2, 3]),
+            (([1, 2], [3, 2]), [1, 3, 2]),
+        )
+        for (list1, list2), expected in test_values:
+            with self.subTest(list1=list1, list2=list2):
+                self.assertEqual(Media.merge(list1, list2), expected)
+
+    def test_merge_warning(self):
+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter('always')
+            self.assertEqual(Media.merge([1, 2], [2, 1]), [1, 2])
+            self.assertEqual(
+                str(w[-1].message),
+                'Detected duplicate Media files in an opposite order:\n1\n2'
+            )