Page MenuHomePhabricator

Throttle parameters may have an undesired comma inside
Closed, ResolvedPublic

Description

See T203554 for a longer explanation. Throttle count or, more likely, throttle period may have a comma inside (being numerical values). However this may create problems since we currently implode the two values with a comma, thus leading to potential internal failures. The linked task will make us stop imploding such values, but existing ones (if any) must be specifically handled within the code, possibly before merging the patch for T203554 (as the implementation takes the presence of a comma a a sign that parameters were imploded).
When we'll be sure that this won't be a problem, we will be able to go on with the linked task.

Event Timeline

Daimona created this task.Sep 5 2018, 5:24 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 5 2018, 5:24 PM
Daimona updated the task description. (Show Details)Sep 7 2018, 4:00 PM
Daimona added a comment.EditedSep 7 2018, 4:12 PM

AFAICS, having a comma is only a problem if it's inside the throttle count. In several points of the code we do something like

list( $count, $period ) = explode( ',', $parameters );

And if, for instance, the specified count is "3,5" and the specified period is "300", the variables from the example above will be $count === 3 and period === 5. Now, this is really, really unlikely to happen. First because the action count is naturally an integer. Second because it could only happen where the comma is the decimal separator. In any case, having a non-integer is totally wrong. I guess values with commas can be just rounded to the closest integer, and we'd better do it inside the maintenance script for T203336.

EDIT: Actually, prior to the OOUI switch, trying to put a comma inside those fields produced some kind of errors, and throttle parameters weren't saved. Given this, I'll just add a check to the mentioned script for commas, throwing a fatalError if parameters aren't fine.

Change 457086 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Fix throttle validation to avoid DB query error

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

Change 457086 abandoned by Daimona Eaytoy:
Fix throttle validation to avoid DB query error

Reason:
Superseded by I7831dbb0bab55807392ac1f7915d6cb0cb713593. The few things of the maintenance script that we still need will be included inside the one in Iee56f4726337a42a04e7912e1ee67738fac0b488

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

Daimona claimed this task.Sep 12 2018, 9:21 AM

Change 459368 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Change throttle selector to restore old functionality, overall improvement

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

Change 468007 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@REL1_32] Change throttle selector to restore old functionality, overall improvement

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

Change 459368 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Change throttle selector to restore old functionality, overall improvement

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

Change 459245 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add normalizeThrottleParameters script to update.php

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

Change 468007 merged by Daimona Eaytoy:
[mediawiki/extensions/AbuseFilter@REL1_32] Change throttle selector to restore old functionality, overall improvement

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

(Is this done now?)

Change 459245 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add normalizeThrottleParameters script to update.php

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

Change 459245 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add normalizeThrottleParameters script to update.php

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

Daimona closed this task as Resolved.Jul 17 2019, 1:43 PM
Daimona removed a project: Patch-For-Review.