Page MenuHomePhabricator

Make external variables true booleans
Closed, ResolvedPublic3 Story Points

Description

Currently, AF uses 3 external vars that are supposed to be booleans but are actually integers (0 and 1), and their documentation (copied & pasted below) is pretty cryptic:

VariableDocComment
user_blocked1 for blocked registered users; null for unregistered users.should be true if the user is blocked, false otherwise, without registered/anonymous distinction. If this isn't possible, then true-false-null
user_mobile1 for mobile users.True for mobile users, false otherwise
tor_exit_node0, 1 (only available if TorBlock is installed)True for mobile users, false otherwise

This shouldn't be too hard to do, however I'm leaving it here for two reasons: first, there are several variable-related patches around and I don't want too many of them. Second, as usual, this would require a query to ensure that we won't break any use case. The only breakable ones that I see are:

  • Triple-comparing the variable.
  • Almost any use of user_blocked due to the value change.

Since I don't think these variables are widely used, a query for user_blocked|user_mobile|tor_exit_node should provide a relatively tiny amount of data to analyse.

Event Timeline

Daimona created this task.Apr 17 2018, 6:04 PM
Restricted Application added subscribers: Scoopfinder, Aklapper. · View Herald TranscriptApr 17 2018, 6:04 PM
Huji added a subscriber: Huji.Apr 17 2018, 6:29 PM

Isn't it the case that true == 1 and false == 0 as far as AbuseFilter parser handles it? If so, then I don't think the use cases of user_blocked will be impacted because they effectively check if it is equal to 1 or not, and converting to either true/false or true/false/null will not change that (it will, if they use the "identical" operator, which you call "triple-compare", but you already have a bullet point for it).

Huji triaged this task as Low priority.Apr 17 2018, 6:29 PM
Huji set the point value for this task to 3.

Hmmm yeah, true. My doubt was about null/false, partly because if we remove registered/anon distinction the variable will change behaviour for unregistered users (won't always be null). But probably yeah, the first bullet is enough.

So, I started looking at this. The change will require 3 different patches: one in TorBlock, one in MobileFrontend and one in AF itself. However, they should be one-line changes. I also tried the change for TorBlock but I'm unsure if it works, due to T190653: in /examine I still see "1" or empty, which isn't enough to tell if the value is actually a boolean. I'll do more testing in the afternoon.

Change 427363 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/TorBlock@master] Pass a true boolean to AbuseFilter

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

Daimona claimed this task.Apr 18 2018, 11:58 AM

WOW! AbuseFilter will never stop surprising me. I just discovered that user_blocked and user_mobile are already booleans. Of course, since in /details they were displayed as 1/empty and the docs explicitly mentioned numbers, this wasn't immediately clear. Also, the behaviour of user_blocked isn't easy to change since it's directly generated inside the User class.
So basically we only need to change tor_exit_node (patch above), which should still be queried.

I examined every result of the query and concluded that no problem will come out due to the lack of ===. There are only two cases (see comment on P7123) where == 1 is used; they will still work, but maybe we should suggest changing them in order to better reflect the boolean nature of the variable. Now we just have to find someone with +2 rights on TorBlock, since AFAIK that extension is short of mantainers.

Change 427363 merged by jenkins-bot:
[mediawiki/extensions/TorBlock@master] Pass a true boolean to AbuseFilter

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

Legoktm closed this task as Resolved.May 29 2018, 4:34 AM
Legoktm added subscribers: Parent5446, Legoktm.

Now we just have to find someone with +2 rights on TorBlock, since AFAIK that extension is short of mantainers.

Feel free to add me as a reviewer in the future. @Parent5446 isn't as active, but usually prompt on code reviews :)

Thanks @Legoktm :-) This task should be solved, although T190653 will confirm that, or show other similar problems