Page MenuHomePhabricator

A wrong $2 parameter is passed to the flow-delete-post-content message
Closed, ResolvedPublic


flow-delete-post-content in english is:

This comment was deleted by $1 ([$2 history])

$2 seems to expect to be a URL, but instead it gets a USERNAME (talk | contribs)

See example in

Event Timeline

Ciencia_Al_Poder raised the priority of this task from to Needs Triage.
Ciencia_Al_Poder updated the task description. (Show Details)
Ciencia_Al_Poder added a subscriber: Ciencia_Al_Poder.
Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptMay 1 2015, 11:04 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 208843 had a related patch set uploaded (by Catrope):
[WIP] Pass correct $2 param to flow-{hide,delete,suppress}-{post,title}-content

Catrope set Security to None.May 4 2015, 11:33 PM
Catrope added a subscriber: matthiasmullie.
Catrope added a subscriber: Catrope.

As far as I can tell this history link feature never worked. I fixed how the $2 parameter is passed, but now it just ends up being interpreted as raw wikitext instead. I'm not sure that having links in edit summary renderings is even possible. Matthias?

Change 208930 had a related patch set uploaded (by Matthias Mullie):
Get rid of convoluted getContent failsafe

@Catrope: It indeed looks like we disallow HTML in summary (though we could change that if we wanted to)

But it probably doesn't make sense to keep this functionality around: that entire else statement in Templating::getContent is really old and we should get rid of it.
That message is used in flow_post_moderation_state.partial.handlebars, which is displayed instead of the post (if it is hidden/deleted/...) - that's where we want to link to history.

We don't want to link to history from inside history. It just no longer makes sense to have that entire else-statement in there, but a couple of places still use Templating::getContent: AbstractFormatter.php, RevisionFormatter.php & TocTopicListFormatter.php
I just submitted to remove that elaborate failsafe inside Templating::getContent that was generating these incorrect messages.

Now, @DannyH, @Quiddity: what do we want to display in instances like this where content may not be available? (e.g. anons are allowed to see entries about deletion in history, Special:Log, ... but they're not allowed to see content)
See this entry, for example:

  • (cur prev topic) 10:00, 5 May 2015 . . Mlitn (Talk | contribs) deleted a comment on "new topic" (test) . . (-4)‎
  • (cur prev topic) 09:59, 5 May 2015 . . (Talk) commented on "new topic" (THIS SHOULD BE AN EXCERPT OF THE REPLY TEXT BUT THEY'RE NOT ALLOWED TO SEE THAT TEXT NOW) . . (+4)‎

What do we want make that bold excerpt? Nothing at all, a general "deleted", "content inacessible", ...? (the patch I just submitted would simply show no text at all)
It used to repurpose "This comment was deleted by $1 ([$2 history])", but linking to history from history makes no sense. And we don't support links from there. And it could be confused with real reply text.

I would be okay with showing nothing there.

(cur prev topic) 09:59, 5 May 2015 . . (Talk) commented on "new topic" . . (+4)‎

Change 208930 merged by jenkins-bot:
Get rid of convoluted getContent failsafe

@DannyH: The annoying thing is: i18n messages don't support conditionals, so we can't do "if there is no text, then don't show parentheses either"

Right now, it'll display this (notice the empty parentheses):
(cur prev topic) 09:59, 5 May 2015 . . (Talk) commented on "new topic" () . . (+4)‎

We could come up with another solution (probably introduce a second message, without parentheses) but it seems like it may be more work than it's worth? (since all lists & actions use that same infrastructure)
Let's file another ticket if we really want to fix that too, so we can estimate (I'd guess a 2), or just ignore that edge case and accept these empty parentheses. :)

DannyH triaged this task as Medium priority.May 8 2015, 7:15 PM
DannyH closed this task as Resolved.May 12 2015, 12:12 AM

Yeah, this works for now. We'll probably revisit it when we do a review of all the log items.

Change 208843 abandoned by Catrope:
[WIP] Pass correct $2 param to flow-{hide,delete,suppress}-{post,title}-content