Code

Opened 5 months ago

Closed 5 months 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: 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).

Attachments (0)

Change History (7)

comment:1 Changed 5 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 5 months ago by bmispelon

  • Cc bmispelon added

comment:3 Changed 5 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 5 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 5 months ago by sehmaschine (previous) (diff)

comment:5 Changed 5 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 5 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

comment:7 Changed 5 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.