Page MenuHomePhabricator

Vector should replace rendering from PHP with Mustache
Open, NormalPublic

Description

@Krinkle made a start to extracting rendering from PHP into Mustache but this was reverted due to a lack of discussion. I am raising this task now to have that conversation. While I don't think an RFC is necessary, a discussion is definitely in order. Feel free to edit description/comment.

Acceptance criteria

It's done when the following is true:

  • All HTML construction happens in Mustache.
  • 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. This can easily be verified by generating HTML before and after the patchset and diffing the result (which will be empty)

Motivation

The intention is to formalise the one-direction travel of information. So that once done, it's ParserOutput + User => Vector skin; which only invokes a Mustache template. Removing the indirection of QuickTemplate, and removing the use of inline db queries and variants, would allow us to:

  • More easily maintain Vector by making the source code live in one file that provides a high-level view of the skin's template.
  • Support an API driven frontend (T140664)
    • render logged-in views nearly as fast as logged-out views (not counting Varnish of course)
    • opens up numerous possibilities for doing that last step (data + Mustache) at the edge, in a service, or client-side.
  • Allow skin developers who may not be as familiar with PHP to work with client side technologies - CSS and Mustache (skin boilerplates can be provided for the necessary PHP)
  • Make code more readable by removing the baton passing and confusion around where to put things that occurs between SkinTemplate and Skin.
  • Longer term, skins could be reduced to folders containing just CSS and Mustache templates (see https://github.com/jdlrobson/SimpleSkins/tree/master/skins/SimpleSkinVector) making skin development incredibly easy! See also T217158: RFC: Skin templating

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 26 2019, 6:43 PM

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

Jdlrobson triaged this task as Normal priority.Feb 26 2019, 6:44 PM
  • 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.
Jdlrobson updated the task description. (Show Details)Feb 26 2019, 7:17 PM
Jdlrobson added a subscriber: Skizzerz.
Jdlrobson updated the task description. (Show Details)Feb 26 2019, 7:20 PM

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?

Tgr added a subscriber: Tgr.Mar 10 2019, 5:49 AM

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.

Tgr added a comment.Mar 10 2019, 5:51 AM

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.

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.

Izno added a subscriber: Izno.Mar 12 2019, 11:35 PM
Tgr added a comment.Mar 14 2019, 9:56 PM

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

This comment was removed by Krinkle.
{{#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).