Opened 4 years ago

Closed 3 years ago

#16776 closed Bug (fixed)

jQuery.noConflict(false) in jquery.init.js leaks the admin jQuery into the global namespace

Reported by: anonymous Owned by: julien
Component: contrib.admin Version: master
Severity: Normal Keywords: jQuery admin
Cc: jezdez Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

jQuery.noConflict(true) should be used instead.

Now that django's jQuery object is namespaced to django.jQuery (#12882), it doesn't need to appear anywhere else in the global namespace. Most jQuery plugins overload the jQuery object (documented here: http://docs.jquery.com/Plugins/Authoring ), and putting the admin's object here makes it harder to use a newer version. django apps like django-selectable, which requires a recent jQuery and jQuery UI, are affected. [16415] should be reverted.

Attachments (2)

16776.diff (1.1 KB) - added by robhudson 4 years ago.
dj16776.tar.gz (2.8 KB) - added by anonymous 3 years ago.
example project

Download all attachments as: .zip

Change History (17)

comment:1 Changed 4 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

For context, noConflict(true) and the namespacing to django.jQuery appeared before releasing 1.2, and [16415] would appear in 1.4.

comment:2 Changed 4 years ago by jezdez

  • Triage Stage changed from Unreviewed to Design decision needed

The problem of using jQuery.noConflict(true) (and which is what was changed to jQuery.noConflict() in r16415) is that it removes any jQuery from the global namespace, even if it's the jQuery shipped with a 3rd party app.

So, with the current state of jQuery.noConflict() you should be able to put another jQuery after Django's jQuery and it should just work because it's overwriting the jQuery object in the global namespace. Unless I misunderstand something the django.jQuery object should then still be the same Django-jQuery.

comment:3 Changed 4 years ago by anonymous

jQuery.noConflict(true) actually restores the previous jQuery (from _jQuery). The admin <script> tags include jquery.min.js immediately followed by jquery.init.js . With noConflict(true), the admin has no effect on the global namespace; it will never remove an existing jQuery. I've checked the jQuery source, and the tests for this behavior appeared in https://github.com/jquery/jquery/commit/1ac9d6fbeed214d59ea5c31a87ff2c19bb08dbc8 (before jQuery 1.2.2).

By the way, extrahead customizations appear before the admin script tags.

Changed 4 years ago by robhudson

comment:4 Changed 4 years ago by julien

  • Triage Stage changed from Design decision needed to Ready for checkin

I've talked with robhudson about this at the sprints. We could not find any specific case where the admin would not have played nicely with third-party plugins prior to [16415]. We do see, however, how leaving jQuery in the base namespace could potentially break plugins. jezdez did commit [16415] to solve a real problem but unfortunately cannot remember the exact details. For these reasons, we should revert the change as a cautionary measure and prevent potential regressions in 1.4. This is not to say that [16415] was necessary invalid -- if one can provide a specific case where this is an issue, please do yell!

comment:5 Changed 4 years ago by jezdez

  • Triage Stage changed from Ready for checkin to Design decision needed

Thanks, but all I need to do is to actually search my chat logs for the specific issue since I wasn't imagining things.

comment:6 Changed 4 years ago by jezdez

  • Owner changed from nobody to jezdez

comment:7 Changed 3 years ago by jezdez

Ah, another piece of the puzzle, this is what had in mind: http://groups.google.com/group/django-developers/browse_thread/thread/c91da363ef335629

comment:8 Changed 3 years ago by jezdez

  • Owner jezdez deleted

Removing myself again since I simply don't know what problems cause r16415 in detail to decide what's best.

comment:9 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:10 Changed 3 years ago by anonymous

  • Owner set to julien

In the google groups post above, tyrion-mx suggests that the django-admin jQuery could overwrite the jQuery object as well.
This was asked for convenience, so that using jQuery plugins doesn't require a copy of jQuery. However that is a bad idea, because it ties the jQuery version to whatever Django ships (which is quite old due to release cycles, and already incompatible with django admin addons), and removes control from users of the django admin. It would break things if it appeared in a release.

Which is why I'll reiterate that django's jQuery should be self-contained, jQuery.noConflict(true) should be used, and the attached patch should be applied.

(There are some incorrect assumptions in tyrion-mx's post: {% block extrahead %} customisations, which appear before the admin jQuery, were working correctly before r16415, because noConflict(true) does not remove the jQuery object, but rather restores it.)

comment:11 Changed 3 years ago by julien

  • Owner julien deleted

@anonymous: I tend to agree with you, however what we need right now is some concrete examples of how/why things would break with the current situation. Could you try to upload some code sample or maybe a sample project (in a zip file) concretely illustrating the problem that you're talking about?

Also, please do not assign tickets to other people unless they've specifically asked you to ;-)

Changed 3 years ago by anonymous

example project

comment:12 Changed 3 years ago by anonymous

Instructions for testing:

./manage.py syncdb
./manage.py runserver

Visit http://localhost:8000/admin/auth/user/

In a js console, display: jQuery.fn.jquery

Try it with django 1.3.1, django 1.4pre, and with 16776.patch applied.

comment:13 Changed 3 years ago by anonymous

  • Has patch set

comment:14 Changed 3 years ago by julien

  • Easy pickings unset
  • Triage Stage changed from Design decision needed to Accepted

Thanks for putting this sample project together. It appears quite clearly that currently the admin simply overrides any user-supplied jQuery with is own embedded one. I've talked through this with jezdez on IRC, and we'll proceed by reverting the change and adding some documentation.

comment:15 Changed 3 years ago by julien

  • Owner set to julien
  • Resolution set to fixed
  • Status changed from new to closed

In [16967]:

Fixed #16776 -- Fixed a regression introduced in r16415 which caused Django's embedded jQuery to overwrite any pre-existing values of window.jQuery in the global namespace. Many thanks to Rob Hudson, Jannis Leidel and "anonymous" for the help resolving this issue.

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