#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: bmispelon 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 Changed 21 months ago by bmispelon

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

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 Changed 21 months ago by bmispelon

  • Cc bmispelon added

comment:3 Changed 21 months ago by bmispelon

  • Component changed from Uncategorized to contrib.admin
  • Easy pickings set
  • Patch needs improvement set
  • Summary changed from SelectFilter2.js: quickElement() is missing third argument to Clean up misuse of null in quickElement
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 21 months ago by sehmaschine

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

  • Use '!==' to compare with ' '
  • Use '!==' to compare with 'null'
Last edited 21 months ago by sehmaschine (previous) (diff)

comment:5 Changed 21 months ago by bmispelon

  • Has patch set
  • Patch needs improvement unset

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

comment:6 Changed 21 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

comment:7 Changed 21 months ago by Baptiste Mispelon <bmispelon@…>

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

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