Opened 13 years ago

Closed 13 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 Phalip
Component: contrib.admin Version: dev
Severity: Normal Keywords: jQuery admin
Cc: Jannis Leidel 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 Rob Hudson 13 years ago.
dj16776.tar.gz (2.8 KB ) - added by anonymous 13 years ago.
example project

Download all attachments as: .zip

Change History (17)

comment:1 by anonymous, 13 years ago

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

comment:2 by Jannis Leidel, 13 years ago

Triage Stage: UnreviewedDesign 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 by anonymous, 13 years ago

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.

by Rob Hudson, 13 years ago

Attachment: 16776.diff added

comment:4 by Julien Phalip, 13 years ago

Triage Stage: Design decision neededReady 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 by Jannis Leidel, 13 years ago

Triage Stage: Ready for checkinDesign 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 by Jannis Leidel, 13 years ago

Owner: changed from nobody to Jannis Leidel

comment:7 by Jannis Leidel, 13 years ago

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 by Jannis Leidel, 13 years ago

Owner: Jannis Leidel removed

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

comment:9 by Jacob, 13 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:10 by anonymous, 13 years ago

Owner: set to Julien Phalip

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 by Julien Phalip, 13 years ago

Owner: Julien Phalip removed

@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 ;-)

by anonymous, 13 years ago

Attachment: dj16776.tar.gz added

example project

comment:12 by anonymous, 13 years ago

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 by anonymous, 13 years ago

Has patch: set

comment:14 by Julien Phalip, 13 years ago

Easy pickings: unset
Triage Stage: Design decision neededAccepted

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 by Julien Phalip, 13 years ago

Owner: set to Julien Phalip
Resolution: fixed
Status: newclosed

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