Page MenuHomePhabricator

Improve the way variables are dumped
Open, NormalPublic

Description

Currently, we have two cases where AF dumps variables: the first is in /examine and /details available for every edit, where all generated variables are dumped in a table. The other is when you evaluate a filter in /tools, where the result (which is more or less a variable) is dumped. The trouble is that many variables aren't clear: for instance, when the filter evaluates to false you get an empty div element, when it evaluates to true you get "1" and arrays are displayed everywhere as imploded with linebreaks. My proposal is to give different dumps, maybe resemblant to PHP's var_dump, where you get a graphic representation of the variable, together with its type.

Event Timeline

Daimona created this task.Mar 25 2018, 4:29 PM
Restricted Application added subscribers: Scoopfinder, Aklapper. · View Herald TranscriptMar 25 2018, 4:29 PM
Daimona triaged this task as Low priority.Mar 25 2018, 4:29 PM

Change 421966 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Improve var dumping in /details and /examine

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

Change 422151 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Use SyntaxHighlight (if available) to show formatted vars

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

Huji added a subscriber: Huji.EditedMar 30 2018, 2:42 AM

421966 changes the output for something like added_lines from this:

TEXT

to this:

array (
  0 => 'TEXT',
)

This is technically more accurate, but visually less useful to the users.

@Huji Actually, it is useful: suppose you want to check for equality with added_lines. With the actual dumping you may think it would be enough to do "added_lines === 'TEXT'", which of course will fail because it's an array. So, IMHO users should clearly see when a variable is an array, even if it has a single element.

Huji added a comment.Mar 30 2018, 3:07 PM

It may be useful; but I would like more people to discuss this before it goes into prod.

@matej_suchanek @MaxSem I added you as reviewers. Do you think it would be appropriate to break our years-old habit in how we see these variables (see T190653#4093632 for example)?

@Huji of course, it's always better to hear thoughts. I personally couldn't see anything bad in showing technically accurate info, which may help users in understanding some AF weirdness. It's enough to cite arrays, I'd bet few people know which variables are arrays and how to handle them; a reason to this is probably the fact that the method used for distinguishing array is... Putting \n's between elements :-)

I'm quite reserved about this change. Is it really appropriate to make AF maintainers rely on PHP stuff? AF arrays are not really like those of PHP.

@matej_suchanek Of course, but they should be quite similar. Anyway, above all, IMHO it's still much better than relying on strings with linebreaks. We may try to find a better way to dump variables, but linebreaks are quite ridicoulous.

Adding as subscribers another couple of people active in AF workboard.

From the description, I was thinking in a presentation similar to this:
https://meta.wikimedia.org/wiki/Schema:AccountCreation

Personally, I would prefer to have accurate information, making it clear when a variable is an array (even if it has a single element).

Summing up, what's the current situation? Should we avoid var_export for 1-element arrays? Or even something totally different? I feel like a change like this is strongly needed: especially when debugging with /tools, sometimes is really tricky to decipher the output, especially with array and false: try to put in [false, false, false] and click evaluate to see how useful is the current dumping system :-)

Daimona claimed this task.Apr 10 2018, 5:38 PM
Daimona raised the priority of this task from Low to Normal.Oct 5 2018, 11:33 AM

I'm abandoning the second part, i.e. using Geshi highlight, and this is the reason.
When dumping variables we have two possibilities: 1-escape the variable 2-don't escape. Escaping here is only intended as PHP-escaping (not HTML), so for instance the AbuseFilter string 'foo"\'bar would be 'foo\"\\\'bar' in PHP (reminder: outer quotes are added when formatting the variable to make clear that it's a string). However, if we escape it we end up with clogged logs, and users may get confused: for instance, all bold/italic wiki-markup inside text variables would be escaped (The '''foo''' => The \'\'\'foo\'\'\'), and this would surely be a problem for whoever wants to read the logs. Instead, we may choose not to escape it, which is what we're doing right now. However, not escaping means that the outputted code is NOT valid code. Or, at least, it's not what it's meant to be: if we have the string 'foo'bar' (where the outer single quotes are added for formatting), in PHP this is the string 'foo' followed by the identifier bar , followed by a single quote. But since in PHP this is not a single string, Geshi won't highlight it as such, thus creating bizarre results.
For these reasons, I chose to kept easier to read variables, renouncing to colors.

Change 422151 abandoned by Daimona Eaytoy:
Use SyntaxHighlight (if available) to show formatted vars

Reason:
See T190653#4664221.

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

Change 421966 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Improve var dumping in /details, /examine and /tools

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