Page MenuHomePhabricator

Filter IDs and log entry IDs should not be num-formatted
Open, Needs TriagePublic

Description

When reviewing an AbuseFilter log entry, I noticed its ID was num-formatted (i.e. separators between groups of digits).
This doesn't make sense here, as it is an identifier (just like a revision ID), not an amount.

When looking at the code, I noticed filter IDs were also num-formatted in some places.
(this issue easily went unnoticed, as there are usually less than 1,000 filters)

Event Timeline

Change 965462 had a related patch set uploaded (by Gerrit Patch Uploader; author: Anne Haunime):

[mediawiki/extensions/AbuseFilter@master] Filter IDs and log entry IDs should not be num-formatted

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

Uploaded patchset 3, which takes care of occurrences of formatNum() that were remaining.

It was a bit tricky:

  • LinkRenderer's makeLink() and makeKnownLink() expect parameters to be of type string (or HtmlArmor or null).
  • exposed formatValue() public methods are expected to return type string.
    • (btw, notice one definition uses $row->afh_filter while the other two use intval( $value ), maybe it could be unified)
  • some messages (namely this one and this one) end up with redundant variables ($2/$3 now same as $1).

Alternatively, we may play it safe for now: keep filter IDs num-formatted, and just cherry-pick the 2 changes for log entry IDs (see in SpecialAbuseLog.php).

@matej_suchanek I took the liberty of subscribing you here, as I mainly comment on Phabricator :)

Arguably, filter IDs could be considered as their "name". For instance, on frwiki we often refer to the filters just by their numbers.

I uploaded patchset 4, which keeps the filter IDs num-formatted, and only removes the num-formatting of log entry IDs, which undoubtedly should not be num-formatted, just as revision IDs.

It's a bit hard to follow the change from the code, can I see which places this tries to change in a screenshot?

For example for log entry id on the following screenshot,

image.png (198×552 px, 28 KB)

Sure thousands separator is extra but num formatting is nicer to have as it along text. Where possible please don't remove num format entirely but just disable the num separator.

Change #965462 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Log entry IDs should not have thousands separators

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