Last modified: 2014-11-17 10:35:09 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T46459, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 44459 - jqueryMsg should not parse plain messages as HTML
jqueryMsg should not parse plain messages as HTML
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Internationalization (Other open bugs)
1.21.x
All All
: High major with 2 votes (vote)
: 1.21.0 release
Assigned To: Matthew Flaschen
:
: 44752 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-29 09:44 UTC by Kunal Mehta (Legoktm)
Modified: 2014-11-17 10:35 UTC (History)
24 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Kunal Mehta (Legoktm) 2013-01-29 09:44:16 UTC
See https://en.wikipedia.org/w/index.php?title=User_talk:Sandipgshinde&action=history: 

(cur | prev) 08:56, 29 January 2013‎ Σ (t | c)‎ . . (1,161 bytes) (+1,161)‎ . . (pagetriage-del-talk-page-notify-summary: Parse error at position 44 in input: Notifying author of deletion nomination for $1)

There were a few other edits that Σ made which had the same broken edit summary.

His browser/OS info:

"Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20100101 Firefox/14.0.1"
Comment 1 Antoine "hashar" Musso (WMF) 2013-01-29 09:45:56 UTC
The message pagetriage-del-talk-page-notify-summary is apparently only used in the javascript side:

modules/ext.pageTriage.views.toolbar/ext.pageTriage.delete.js:719:
 'summary': mw.msg( 'pagetriage-del-talk-page-notify-summary', pageName ),

Maybe pageName is an object or mw.msg has some serious bug :-D
Comment 2 Nischay Nahata 2013-01-29 12:23:02 UTC
The only assignment I see is
var pageName = new mw.Title( mw.config.get( 'wgPageName' ) ).getPrefixedText();

which returns a string, not sure what the problem is.
Comment 3 Alex Monk 2013-01-29 22:25:42 UTC
mw.msg( 'pagetriage-del-talk-page-notify-summary', "User talk:Sandipgshinde" );
"pagetriage-del-talk-page-notify-summary: Parse error at position 44 in input: Notifying author of deletion nomination for [[$1]]"
Comment 4 Alex Monk 2013-01-30 00:53:29 UTC
CCing Krinkle - I think this is a bug in mw.msg.
Comment 5 Matthew Flaschen 2013-01-30 22:53:26 UTC
Hmm, it looks like this is a result of letting jqueryMsg take all messages containing a square bracket.

This was part of my change eaaec38d6fdba01b30d051711dc84865fbe388ad (before it did the same, but only for curly braces).

mw.msg (as used here) is plain by default.  But there is a FIXME in mediawiki.js that plain and parse are wrongly treated exactly the same.

I think it is time to fix this.  My initial idea is:

1. Just pass the format to parser()
2. If it is plain, fall back to old mw.msg, which should work fine for this, and similar cases where HTML is not wanted.

Later, jqueryMsg could be extended to support plain parsing.
Comment 6 Matthew Flaschen 2013-01-30 22:54:02 UTC
Plain parsing in jqueryMsg could be useful if you don't want HTML but do want, e.g. int:, plural:, etc. support.
Comment 7 Michael M. 2013-01-31 09:20:54 UTC
There are similar reports for the WikiEditor extension, which might be duplicates of this one: Bug 44215, Bug 44195, Bug 42107.
Comment 8 Michael M. 2013-01-31 09:36:13 UTC
Another situation where this occurs: Go to https://de.wikipedia.org/wiki/Kategorie:!Hauptkategorie (or any other category page) and click the [+] to open a subcategory. It opens, but a "categorytree-collapse-bullet: Parse error at position 0 in input:" is inserted.
Comment 9 Matthew Flaschen 2013-02-01 04:04:35 UTC
Proposed fix and tests at https://gerrit.wikimedia.org/r/#/c/47044/
Comment 10 Matthew Flaschen 2013-02-01 08:10:21 UTC
It doesn't affect the current fix, but part of my comment (6) above is incorrect.

If we want to support the curly braces but not HTML, I believe that would be text, not plain.  From https://www.mediawiki.org/wiki/Manual:Messages_API#Format:

"wfMessage()->text(); transforms the message text (MessageCache::transform() which transforms all '{{}}')".

'plain' (which the fix is regarding) only has parameter substitution.
Comment 11 Michael M. 2013-02-01 08:56:27 UTC
(In reply to comment #9)
> Proposed fix and tests at https://gerrit.wikimedia.org/r/#/c/47044/

As I don't have a gerrit account, I just comment here: The test with categorytree-collapse-bullet seems useless, as the value used on en.wp doesn't contain any critical characters. Use the default content [<b>−</b>] instead.
Comment 12 Matthew Flaschen 2013-02-01 09:10:31 UTC
True.  I realized that a while after I posted the change.  I will fix it later today, though the pagetriage-del-talk-page-notify-summary covers similar ground.

You can sign up for Gerrit/Labs at https://labsconsole.wikimedia.org/wiki/Help:Getting_Started#Create_a_User_Account .  It no longer requires manually asking anyone.
Comment 13 Matthew Flaschen 2013-02-01 21:31:54 UTC
Done in patch set 3, so please review.
Comment 14 Bartosz Dziewoński 2013-02-04 17:32:17 UTC
Raising importance, as this is highly visible.
Comment 15 Nischay Nahata 2013-02-07 16:16:09 UTC
*** Bug 44752 has been marked as a duplicate of this bug. ***
Comment 16 Matthew Flaschen 2013-02-09 01:31:50 UTC
Merged.
Comment 17 Bartosz Dziewoński 2013-02-09 12:37:15 UTC
This is causing breakage all over [https://pl.wikipedia.org/w/index.php?title=Dyskusja_wikipedysty:Matma_Rex&oldid=34672851#Parse_error_at_position...], so I'm getting these changes backported.
Comment 18 Niklas Laxström 2013-02-10 11:01:40 UTC
Reopening. The fix is also causing breakage all over and needs to be replaced with a better fix.

mw.msg() is expected to parse plural in existing code.
Comment 19 Matthew Flaschen 2013-02-10 12:15:42 UTC
mw.msg is a shortcut for mw.message(...).plain(), and contrary to what the old mediawiki.js comment suggested, plain is not supposed to support {{plural}}.  See https://www.mediawiki.org/wiki/Manual:Messages_API#Format and the source code of Message.php.

plain "returns the message text as-is; only parameters are substituted."

Code that needs {{-transformation on the client side should use 'parse' ('text' is not currently supported in JS):

mw.message(key).parse()

There is no good way (other than format, which is designed for this) to figure out how to parse message text like "[<b>−</b>]".

If you give the affected messages, I can help change them to use parse.
Comment 20 Matthew Flaschen 2013-02-10 13:01:58 UTC
Í've uploaded a Gerrit to update some tests that were using plain for plural and gender:

https://gerrit.wikimedia.org/r/#/c/48349/

All I had to do was change the format, not the expected output.

As we keep developing jqueryMsg and client-side message parsing, I don't think we can have this inconsistency (client and server) in the behavior of plain.

In general we should bring client-side message parsing in line with the server behavior.
Comment 21 Matthew Flaschen 2013-02-10 13:02:27 UTC
Reopening until that is merged.
Comment 22 Niklas Laxström 2013-02-10 17:46:17 UTC
We are on the right track here.

"mw.msg is a shortcut for mw.message(...).plain()"

Here I disagree. mw.msg should be a shortcut to mw.message().text(), just like what wfMsg was in the PHP side. There is actual code out there depending on this behavior.

I am sorry that we have got into this mess, but I think there is only way out here, and that is, like you said, to make the JavaScript side match PHP side.

This means:
* mw.message.plain() is now working correctly
* mw.message.parse() output should be safe for direct inclusion in html, parsing {{ and link syntax
* mw.message.text() should be introduced and it should function like mw.msg used to function: expand {{ syntax
* mw.msg should be aliased to mw.message.text()
* mw.message.escaped() should be the same as mw.message.text() with html escaping
* fix the calls that assume something else than what was stated above
Comment 23 Nemo 2013-02-10 19:40:20 UTC
Assigning to last patch's owner.
Matthew, are you going to do what above?
Comment 25 Matthew Flaschen 2013-02-11 13:25:28 UTC
> "mw.msg is a shortcut for mw.message(...).plain()"

I was saying how mw.msg behaves now.  It is currently plain.

I think this Niklas's proposal in comment 22 is a reasonable approach.  I'm going to start implementing it.

The main part that be people need to be be careful about is the mw.msg default.  wfMessage (the now-recommended API on the server-side) defaults to 'parse'.

We could change it to that, but that would lead to the same short-term problems (e.g. categorytree-collapse-bullet parsing wrong).

So I think we should go with 'text', but recommend people use the long-form approach going forward.  This is already recommended for PHP at https://www.mediawiki.org/wiki/Manual:Messages_API#Format ("It is recommended to specify the output format at the end of wfMessage( 'message-key' )).
Comment 26 Matthew Flaschen 2013-02-11 15:50:59 UTC
Daniel Werner has put in a Gerrit reverting the change to use plain (https://gerrit.wikimedia.org/r/#/c/48456/).

Since there will be breakage either way (until I'm done the solution proposed in comment 22), I don't have a strong preference.
Comment 27 Jon 2013-02-11 22:24:33 UTC
*** Bug 44851 has been marked as a duplicate of this bug. ***
Comment 28 Matthew Flaschen 2013-02-12 06:15:09 UTC
The proposed fix/solution is at https://gerrit.wikimedia.org/r/#/c/48601/ .  Please review.
Comment 29 Daniel Friesen 2013-02-12 06:26:36 UTC
Note that I actually have some unreviewed changes for jqueryMsg that have been waiting to be merged in. Someone should check if the changes I made make any effect on this bug.
https://gerrit.wikimedia.org/r/#/c/4051/
https://gerrit.wikimedia.org/r/#/c/4055/
Comment 30 Matthew Flaschen 2013-02-12 06:40:14 UTC
I do wish I knew about those earlier, particularly text mode.

However, even for the newer one, it's been almost three months since anyone followed up in any way.

There has been significant work since then, even before this, so merging would be quite challenging.

So I think 48601 should go first.  You can then rebase those changes that make sense, and I'll be glad to help review.
Comment 31 Andre Klapper 2013-02-12 10:40:46 UTC
Potential dups/deps: bug 44885, bug 44832.
Comment 32 Nemo 2013-02-12 10:49:30 UTC
Thank you Matthew. Could you clarify why you removed 1.21.0 milestone? As far as I can see, we can definitely *not* ship a stable release with this bug.
Comment 33 Sumana Harihareswara 2013-02-12 14:05:58 UTC
Gerrit change #48601 has now been merged.
Comment 34 Matthew Flaschen 2013-02-13 01:33:30 UTC
I didn't mean to change the milestone.

I'm marking this fixed.  msg now uses text, which should work in almost all cases.  In the remaining ones, we may have to change it to use a different format.

CC me if you think that's the case, and I can help assist if needed.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links