Change 493093 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Start extracting rendering from PHP into Mustache

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

• There is no change to the HTML output from MediaWiki, the Vector skin, nor any other skin. There is zero impact on end-users, gadget authors, etc.
• There is no change to what MediaWiki's skin system supports. No impact on skin/extension developers.

The purpose here is:

• For the maintainers of the Vector skin to make it easier for them to do their job, using methodologies they prefer and choose within the limited set of technologies already available and approved for use in MediaWiki (this is domain of Reading Web currently). Mustache is such a technology. Already having been used in Minerva/MobileFrontend for several years, as well as in MediaWiki core on various special pages.
• To improve the response time of MediaWiki for logged-in users of WMF wikis, e.g. when generally browsing the site on articles, history, talk pages etc. This is a long-term goal of mine within the Performance Team. Note that I intend to meet this goal entirely within MediaWiki PHP. No separate services, Node.js, or anything like that. While this direction certainly makes that option more realistic in the future, it isn't part of my work here, and won't stop us from getting a decent 2X or 3X response time improvement out of plain MediaWiki core with Vector skin.

I opened the RFC T217158 due to the above change in gerrit that was made and reverted. I believe that an RFC is necessary for the expanded scope of making all skins use a template engine and have a mostly-consistent DOM between them. This task fits within the scope of the RFC I opened, and if the RFC is accepted, I would love to work with @Krinkle at moving this task forward in a way that serves the short-term needs presented here while still being usable for the long-term goals mentioned in the RFC.

The Vector skin has a PHP function that concatenates a few strings, and applies htmlspecialchars() to some of them.

The patch @Jdlrobson submitted moves some those trivial string operations to a Mustache file, which then does the same thing.

This does not require an RFC.

This does not require an RFC.

I agree, but I am keen to hear specifically from @Isarra and @ashley who were a little anxious about the initial patch. Now the scope of this is well defined to these concerns still exist?

How does this affect downstream skins? AFAIK Vector is popular as a "parent" skin; will modifying it become harder (or easier)? More specifically, will there be a mechanism to override a template?
How does it affect security? (One reason I'm not a fan of Mustache - {{x}} vs. {{{x}}} to differentiate between safe parameters and potential XSS vectors is as broken a notation as you can get.)
Maintainability-wise it is a pretty clear win, although I think you could achieve it with plain old PHP templates as well.

In T217172#5013439, @Tgr wrote:

How does it affect security? (One reason I'm not a fan of Mustache - {{x}} vs. {{{x}}} to differentiate between safe parameters and potential XSS vectors is as broken a notation as you can get.)

OTOH the html- naming convention is nice.

In T217172#5013439, @Tgr wrote:

How does this affect downstream skins? AFAIK Vector is popular as a "parent" skin; will modifying it become harder (or easier)? More specifically, will there be a mechanism to override a template?

This is basically why we'd like to establish a more clear path forward for skins in general with the all-skins RfC.

How does this affect downstream skins? AFAIK Vector is popular as a "parent" skin; will modifying it become harder (or easier)? More specifically, will there be a mechanism to override a template?

Why would this change? We are talking about replacing string stitching with templating which seems like a no-brainer to me. Even better, skin developers will be able to replace entire templates rather than having to extend PHP functions.

e.g.

$html = '<div>' . Html::element('span', [],  $bar ) ) . '</div>

becomes

<div><span>{{bar}}</span></div>

I'm still not hearing any strong reasoning not to do this. What are the concrete concerns here? As @Krinkle points out in T217172#4985877 using Mustache is nothing new and the output HTML is the same. It also makes the code easier to work with, and can always be reverted at a later day if anything radical happens to the skin system but if the real grievance here is a dislike of Mustache or template in general, that would be a good thing to bubble up.

It seems the problem with the original merge was the timeframe from submission to merge, however, there has now been 2 weeks, more than enough to provide constructive feedback. I would like to give another week and merge this patch next week so please let me know what these concerns are so that they can be incorporated in the patch as necessary.

Do you have any thoughts on T217158?

This can move forwards without that RFC being settled yet. The work done on this patch set would help provide some of the work that's needed for the linked RFC, but additional changes will be required should the RFC be approved and work begun on it.

I don't see a reason to block this task on the RFC though; it solves a need right now and doesn't conflict with what the overall RFC aims to accomplish either.

All right, then.

In T217172#5018684, @Jdlrobson wrote:

Why would this change? We are talking about replacing string stitching with templating which seems like a no-brainer to me.

Template files and parameter arrays could be used as natural extension points, whereas currently there aren't any. But the RfC is already considering that, so not in the scope of this task I guess.

Change 493093 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Start extracting rendering from PHP into Mustache

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

Possibly introduced T219864: Articles about zero (0) not displaying title in Vector skin

{{#html-title}}<h1 …>{{{html-title}}}</h1>{{/html-title}}

This looks like the problematic line.
{{# is the syntax for conditional

It should be written like so:

{{#hasTitle}}<h1 …>{{{html-title}}}</h1>{{/hasTitle}}

Mustache conflates conditionals with looping based on the type of the thing after the #
So

{{#posts}}{{foo}}{{#posts}}

Would render

{
posts: [ { foo: "test" }, { foo: "test } ]
}

or

{
posts: true,
foo: "hello"
}

@Jdlrobson The problem is not the conditional.

See T219864#5078282 for more details. The conditional is passing as true just fine (only null is falsey in Mustache, empty string and 0 are truthy, as they should be).

T239248 might be a duplicate but adding as subtask so we can check and sign off when that work completes.