Page MenuHomePhabricator

Custom TemplateParser for MobileFrontend and Gather
Closed, ResolvedPublic

Description

The StructuredDiscussions project has yet to experience any issues with LightnCandy as they precompile their templates during development time and write them to disk to be included later, i.e. they don't eval the compiled template code and get bitten by T102937.

@EBernhardson has provided us with a trimmed down version of the Flow TemplateParser class here: https://gist.github.com/anonymous/dd63bccdb955f49a1e76. We need to implement it, test it, and use it in both of the extensions.

  • Create the MobileTemplateParser class using the gist provided
  • Use it in the MobileFrontend extension
  • Use it in the Gather extension

The Gather change is simple: modify the Gather\views\helpers\Template class to use an instance of MobileTemplateParser instead of the TemplateParser class.

We should adopt a similar approach in the MobileFrontend extension: create a helper class, which abstracts away the templating implementation and, initially, make it use the MobileTemplateParser.

Event Timeline

phuedx created this task.Jun 18 2015, 10:49 AM
phuedx raised the priority of this task from to Needs Triage.
phuedx updated the task description. (Show Details)
phuedx added projects: MobileFrontend, Gather, HHVM.
phuedx added subscribers: phuedx, Jhernandez, EBernhardson.
Restricted Application added a project: Readers-Web-Backlog. · View Herald TranscriptJun 18 2015, 10:49 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
phuedx triaged this task as High priority.Jun 18 2015, 10:49 AM
phuedx set Security to None.
phuedx moved this task from Needs Analysis to To Do on the Mobile-Web-Sprint-49-Wayne's-World board.
phuedx claimed this task.Jun 18 2015, 1:39 PM
phuedx moved this task from To Do to Doing on the Mobile-Web-Sprint-49-Wayne's-World board.
phuedx updated the task description. (Show Details)Jun 18 2015, 2:01 PM

Change 219188 had a related patch set uploaded (by Phuedx):
Extend the core TemplateParser

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

If 219188 is merged – we're happy with the approach as well as the code – then we'll need to message out that the $wgGatherRecompileTemplates config variable needs to be set to true in LocalSettings.php or equivalent during development.

Any reason why we‘re not doing it in core? If I‘m not mistaken, MobileFrontend and Gather are the only consumers (or biggest) of Templates in core.

Tested locally with my borked setup and it fixes it, no noticeable issues.
Code looks great.

@bmansurov this is a temporary fix that only affects Gather and maybe MobileFrontend, I'm not sure this fits in core since the idea is to remove it when @EBernhardson's hhvm patch gets merged and derployed.

In any case this extends core's class, so if merged and proven that it works properly in a prod-like environment we could try sending the patch as a new class to core as FileSystemTemplateParser or something like that if it really has interest for core and other extensions. I don't think there's going to be much interest though. Maybe for StructuredDiscussions?

Jdlrobson moved this task from Needs triage to This Sprint on the Gather board.Jun 19 2015, 3:39 PM

Change 219188 merged by jenkins-bot:
Extend the core TemplateParser

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

@bmansurov this is a temporary fix that only affects Gather and maybe MobileFrontend, I'm not sure this fits in core since the idea is to remove it when @EBernhardson's hhvm patch gets merged and derployed.

Well the bug is for code in core, so it should be patched in core. Even if a HHVM patch is deployed, we still need to support older non-hotpatched HHVM versions.

In any case this extends core's class, so if merged and proven that it works properly in a prod-like environment we could try sending the patch as a new class to core as FileSystemTemplateParser or something like that if it really has interest for core and other extensions. I don't think there's going to be much interest though. Maybe for StructuredDiscussions?

...just put it in core to begin with?

@Legoktm the proper fix is the upstream hhvm bug captured in T102937 - when that's fixed we will kill this code. We shouldn't put it in core, since non-hhvm mediawiki instances are not impacted by it
See @Krinkle comments around this approach in https://phabricator.wikimedia.org/T101918#1375707

Jdlrobson closed this task as Resolved.Jun 19 2015, 6:11 PM