Clean up misuse of null in quickElement

[parsch.inc@…]

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).

[Baptiste Mispelon]

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.

[Baptiste Mispelon]

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

[Patrick K]

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

• Use '!==' to compare with ' '
• Use '!==' to compare with 'null'

[Baptiste Mispelon]

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

[Baptiste Mispelon <bmispelon@…>]

In 3ca0815c0b4ddf7dd1fe74839e2c3e8633c3ea31:

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

Thanks to trac user parsch for the report.