Page MenuHomePhabricator

collapsibleTabs code cleanup: null != undefined, undefined variables passed to .data()
Closed, ResolvedPublic

Description

so, in 1.22wmf8, there is a new collapsibleTabs.js ( https://bits.wikimedia.org/static-1.22wmf8/skins/vector/collapsibleTabs.js
)
in line 85 we ask "if ( $settings !== null )".
clearly, this is the wrong question. maybe

if ( typeof($settings) === "object" )

will work better.

as it is, when there is no data, "$setting" is undefined, and the test
$setting != null
returns true, because "null" and "undefined" are two different things.

this cause many things to fail, and in particular, unregistered users lose the "history" tab if they have JS enabled.

peace.


Version: 1.22.0
Severity: minor
Whiteboard: gci2013 https://www.mediawiki.org/wiki/Google_Code-In#Candidate_tasks

Details

Reference
bz50193

Event Timeline

bzimport raised the priority of this task from to Low.Nov 22 2014, 2:04 AM
bzimport set Reference to bz50193.
bzimport added a subscriber: Unknown Object (MLST).
Kipod created this task.Jun 25 2013, 8:02 PM

(In reply to comment #0)

so, in 1.22wmf8, there is a new collapsibleTabs.js (
https://bits.wikimedia.org/static-1.22wmf8/skins/vector/collapsibleTabs.js

It has been moved from the Vector extension with almost no changes, see gerrit change 55524 and gerrit change 56711. This part in particular was not changed (https://gerrit.wikimedia.org/r/#/c/56711/5/modules/jquery.collapsibleTabs.js vs https://gerrit.wikimedia.org/r/#/c/55524/13/skins/vector/collapsibleTabs.js).

So this either actually magically works, or has never worked. (I haven't tested right now.)

[Unlike bug 50196 this isn't critical, as as far as I can see it doesn't break stuff.]

i definitely experienced exceptions caused by this, but this was while chasing bug 50196, which turned out to be, as you noted, unrelated.

i do not have a good reproduction recipe to demonstrate those exceptions, but it is clearly a bug.

i agree that setting it to "critical" maybe was a bit panicky - again, at the time i thought this issue was the source of the 50196 behavior.

peace.

This currently doesn't cause any bad behavior as far as I can see ($settings will never be undefined, apparently), but it's obviously a bug.

There's also something else in this file:

XXX: 'data' is undefined here, should the 'data' from the outer scope have
a different name?
$( this ).detach().prependTo( target ).data( 'collapsibleTabsSettings', data );

It would be nice if someone spent some time trying to understand what all that code is doing.

Adjusting bug summary and metadata.

Change 96918 had a related patch set uploaded by Yamelnychuk:
Fix collapsibleTabs code cleanup: null != undefined & undefined variables passed to .data().

https://gerrit.wikimedia.org/r/96918

Change 96918 merged by jenkins-bot:
Fix collapsibleTabs code cleanup: null != undefined & undefined variables passed to .data().

https://gerrit.wikimedia.org/r/96918

Fixed by Yaroslav as a GCI task, should be all pretty now. kipod, I hope this makes you happy ;)