Page MenuHomePhabricator

FlaggedRevs' checkbox label displayed weirdly in VisualEditor save dialog
Closed, ResolvedPublic1 Story Points

Description

FlaggedRevs' checkbox label is displayed weirdly in VisualEditor save dialog.

Event Timeline

matmarex created this task.Sep 19 2017, 7:10 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 19 2017, 7:10 PM
matmarex claimed this task.Sep 19 2017, 7:11 PM
matmarex triaged this task as Unbreak Now! priority.
matmarex added a subscriber: Catrope.

This is a regression from https://gerrit.wikimedia.org/r/#/c/375020/, as pointed out on the change by @Catrope:

FIXME this causes errors/notices. label-message can either be a string (message key) or a Message object. FlaggedRevs uses the latter, setting the label-message of one of its checkboxes to $this->msg( 'revreview-check-flag-p' )->numParams( $this->article->getPendingRevCount() );
Running something that's either a message key or a Message object through $this->msg() works just fine, but using it as an array index breaks: https://logstash.wikimedia.org/app/kibana#/doc/logstash-*/logstash-2017.09.14/mediawiki?id=AV6CDuKFePsvZ6LqgLXw&_g=h@44136fa . Even if it did automagically __toString() the Message object, that would have the wrong outcome, because we appear to want the message key as the index here, not the rendered message.
This FR checkbox is the only checkbox that uses a Message object, and I fear it might be completely broken by this change. But maybe magic __toString()-ification of the Message object both on this line and in the JSONification of $checkboxesDef makes it work by accident?

Restricted Application added subscribers: Liuxinyu970226, Jay8g, TerraCodes. · View Herald TranscriptSep 19 2017, 7:11 PM

Actually, I'm not sure if this is UBN-worthy? As far as I can tell the checkbox works correctly, it's just a minor display issue. I've got a patch for this ready anyway, it's just a decision of whether to backport/deploy it. @Deskana?

I'm happy to back-port it, but I don't think it's UBN!; Normal, probably.

Change 378985 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] ApiVisualEditor: Fix checkbox label message handling with Message objects

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

Deskana lowered the priority of this task from Unbreak Now! to High.Sep 19 2017, 7:47 PM

Actually, I'm not sure if this is UBN-worthy? As far as I can tell the checkbox works correctly, it's just a minor display issue. I've got a patch for this ready anyway, it's just a decision of whether to backport/deploy it. @Deskana?

Backporting and deploying makes sense. Although it's not "Unbreak Now!", it's still a glitch that'd be nice to have fixed fast.

Deskana set the point value for this task to 1.

Change 378985 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] ApiVisualEditor: Fix checkbox label message handling with Message objects

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

Change 378990 had a related patch set uploaded (by Jforrester; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@wmf/1.30.0-wmf.18] ApiVisualEditor: Fix checkbox label message handling with Message objects

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

Change 378991 had a related patch set uploaded (by Jforrester; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@wmf/1.30.0-wmf.19] ApiVisualEditor: Fix checkbox label message handling with Message objects

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

Change 378990 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@wmf/1.30.0-wmf.18] ApiVisualEditor: Fix checkbox label message handling with Message objects

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

Change 378991 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@wmf/1.30.0-wmf.19] ApiVisualEditor: Fix checkbox label message handling with Message objects

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

Mentioned in SAL (#wikimedia-operations) [2017-09-19T23:56:00Z] <thcipriani@tin> Synchronized php-1.30.0-wmf.19/extensions/VisualEditor/ApiVisualEditor.php: SWAT: [[gerrit:378991|ApiVisualEditor: Fix checkbox label message handling with Message objects]] T176249 (duration: 00m 48s)

Mentioned in SAL (#wikimedia-operations) [2017-09-19T23:57:13Z] <thcipriani@tin> Synchronized php-1.30.0-wmf.18/extensions/VisualEditor/ApiVisualEditor.php: SWAT: [[gerrit:378990|ApiVisualEditor: Fix checkbox label message handling with Message objects]] T176249 (duration: 00m 49s)

Jdforrester-WMF closed this task as Resolved.Sep 19 2017, 11:59 PM
Jdforrester-WMF removed a project: Patch-For-Review.
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptSep 19 2017, 11:59 PM