Opened 10 years ago

Closed 10 years ago

#21445 closed Cleanup/optimization (fixed)

Clean up misuse of null in quickElement

Reported by: parsch.inc@… Owned by: nobody
Component: contrib.admin Version: 1.6
Severity: Normal Keywords:
Cc: Baptiste Mispelon Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

On line 39 of SelectFilter.js the quickElement('div', from_box.parentNode)is missing the third argument – one would expect it to be quickElement('div', from_box.parentNode, '').

This does not lead to an error with django so far, but led to one with django-grappelli (https://github.com/sehmaschine/django-grappelli/issues/405).
I know that this should theoretically be prevented by line 41 of core.js (https://github.com/django/django/blob/master/django/contrib/admin/static/admin/js/core.js#L41) but it failed anyway.

As all other quickElement()s in SelectFilter.js carry a third argument, that argument might be applied to the incomplete one too.

There`s also a pull request on github referring to this (https://github.com/django/django/pull/1922).

Change History (7)

comment:1 by Baptiste Mispelon, 10 years ago

Hi,

While the proposed PR doesn't look like it would hurt, I can't really merge it until I can confirm that it does fix some issue and unfortunately, I can't reproduce the problem locally.

On top of that, two other things bother me:

  • The code of quickElement looks like it'd prevent the issue you're describing.
  • There are other usage of quickElement(foo, bar) in Django's code. If there's indeed a bug, it should be fixed everywhere.

If you could provide a way to reproduce the issue (a testcase would be ideal), it would go a long way towards closing this ticket.

Thanks.

comment:2 by Baptiste Mispelon, 10 years ago

Cc: Baptiste Mispelon added

comment:3 by Baptiste Mispelon, 10 years ago

Component: Uncategorizedcontrib.admin
Easy pickings: set
Patch needs improvement: set
Summary: SelectFilter2.js: quickElement() is missing third argumentClean up misuse of null in quickElement
Triage Stage: UnreviewedAccepted

After some digging, I managed to reproduce the issue locally.

It turns out that technically, the bug is in grappelli, but we can probably improve things on Django's side too.

It all comes down to line 41 of core.js (of which grappelli ships a copy).

Django's version [1]:

if (arguments[2] != '' && arguments[2] != null) {

Grappelli's version [2]:

if (arguments[2] !== '' && arguments[2] !== null) {

Do you see the difference (it took me some time to notice it)?

It turned out that arguments[2] is undefined if not passed, not null.
However, undefined and null are mostly equivalent, but not when using the strict operator ===.

In the end, I think the correct thing to do would be to use undefined instead of null, or even simply to use if(arguments[2]), which I believe should handle all cases.

Consequently, I'll mark this ticket as accepted but I'll change its description a bit to reflect what I said above.

Thanks.

[1] https://github.com/django/djangoblob/master/django/contrib/admin/static/admin/js/core.js#L41
[2] https://github.com/sehmaschine/django-grappelli/blob/master/grappelli/static/admin/js/core.js#L41

comment:4 by Patrick K, 10 years ago

Thanks for checking — we did change this line with grappelli because of jshint:

  • Use '!==' to compare with ' '
  • Use '!==' to compare with 'null'
Last edited 10 years ago by Patrick K (previous) (diff)

comment:5 by Baptiste Mispelon, 10 years ago

Has patch: set
Patch needs improvement: unset

How about this for a fix: https://github.com/django/django/pull/1964 ?

comment:6 by Tim Graham, 10 years ago

Triage Stage: AcceptedReady for checkin

comment:7 by Baptiste Mispelon <bmispelon@…>, 10 years ago

Resolution: fixed
Status: newclosed

In 3ca0815c0b4ddf7dd1fe74839e2c3e8633c3ea31:

Fixed #21445 -- Clean up misuse of null in quickElement.

Thanks to trac user parsch for the report.

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