Page MenuHomePhabricator

A cached server-side HTML template should update when you change a partial template which it includes
Open, HighPublic5 Estimate Story Points

Description

https://gerrit.wikimedia.org/r/#/c/223165/ has a top-level template Skin.mustache that includes the partial template sidebar.mustache using {{>sidebar}}.

I notice on my local wiki if I edit Skin.mustache and view a page with the skin, I see the change to it (good!), but if I edit the partial sidebar.mustache, I don't see the change to it, even if I use ?action=purge (bug!).

Looking at includes/TemplateParser.php in core, it does a simple

// Read the template file
$fileContents = file_get_contents( $filename );

// Generate a quick hash for cache invalidation
$fastHash = md5( $fileContents );

this doesn't notice changes to partials included by $filename.

(This is probably hard to fix. A workaround is to make cosmetic changes to all parent templates that include the edited partial template. The bug and workaround is documented in https://www.mediawiki.org/wiki/Manual:HTML_templates#Caching.)

Developer notes

We'd like to restrict these changes to Vector for the time being to check the performance implications of such a change. We could use a custom template partial validator if necessary or allow the passing of an alternative cache key.

Details

Related Gerrit Patches:

Event Timeline

Spage raised the priority of this task from to Needs Triage.
Spage updated the task description. (Show Details)
Spage added a project: MediaWiki-General.
Spage added subscribers: Spage, kaldari, Jdlrobson.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 18 2015, 8:35 PM
Aklapper set Security to None.
Jdlrobson added a subscriber: ori.Sep 21 2015, 10:34 PM

Less solved this issue with imports. @ori do you remember how we did that?

Jdlrobson triaged this task as Medium priority.Aug 25 2016, 8:47 PM
Jdlrobson moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.

May need to write a spike to explore ways to fix this.

ovasileva lowered the priority of this task from Medium to Low.Sep 28 2016, 4:45 PM
ovasileva added a subscriber: ovasileva.

@Jdlrobson - wondering why you moved this to needs analysis, seems we have a current solution - are there any other news around this?

Someone should go talk to @ori or someone who has similar knowledge (@Krinkle? @EBernhardson) about how we do this for LESS files (which has a similar problem) and flesh this card out with an actionable plan.

Krinkle added a comment.EditedSep 28 2016, 6:50 PM

For LESS, we cache both the CSS output of the compiler and the meta data generated by the compilation process. Part of that meta data is a list of related files. Related files being, other Less files parsed by run-time imports, as well as any mentioned image files (for CSSMin embed).

Then, when considering to use the cached output, we validate the mtime of the main Less file (the entry point), as well as the mtimes of the related files.

Related source code: ResourceLoaderFileModule::compileLessFile

This particular method actually uses md5 hashes instead of timestamps. Either will work for your purpose, though hashes will be more efficient considering that all our source files come from Git, which does not track mtimes. As such, every week when a new WMF branch is created, the mtimes reset to "now" on the app servers.

Thanks @Krinkle! ^ @olga with that in mind this seems to be "triaged but future"

This proposal is selected for the Developer-Wishlist voting round and will be added to a MediaWiki page very soon. To the subscribers, or proposer of this task: please help modify the task description: add a brief summary (10-12 lines) of the problem that this proposal raises, topics discussed in the comments, and a proposed solution (if there is any yet). Remember to add a header with a title "Description," to your content. Please do so before February 5th, 12:00 pm UTC.

Tgr updated the task description. (Show Details)Feb 5 2017, 4:52 AM
Tgr renamed this task from A cached server-side HTML template doesn't update when you change a partial template which it includes. to A cached server-side HTML template should update when you change a partial template which it includes.Feb 5 2017, 9:24 AM
Tgr updated the task description. (Show Details)

I'd recommend this proposal to be open to other solutions (a proposal to solve a problem instead of implementing a specific solution). While this could be considered a bug in the caching logic and could be solved the same way we solved it for LESS and CSSMin, I'd argue that HTML-templates could be considered a very different system and may be better suited using a different approach. For example, by injecting the template instead of hardcoding it in the source.

This would mean that the place where the sub-template is rendered, an HTML (or text) parameter is used instead and the value set to to the expanded template. This could lead to better separation of concerns and better re-use. For general usage, a common interface (function, method or class) may still provide the same interface of a common entry point where needed. This would naturally cache each portion as its own template and therefore invalidate it accordingly.

Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptJun 18 2019, 6:24 PM
Jdlrobson raised the priority of this task from Low to Medium.Sep 10 2019, 3:30 PM
Jdlrobson moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.

This continues to cause friction in software development slowing down our development time. I think investigating this should be prioritised.

Krinkle added a comment.EditedDec 17 2019, 5:18 PM

Using partials in this way with our current on-demand parsing and caching is not supported by upstream LightNCandy and arguably should not have been adopted as such. See T113095#3003037 for how this use case can be accommodated within the current version of TemplateParser as spec'ed and implemented in 2015.

I think it would better utilise our limited resources around Mustache platform maintenance if we pick only one of "recursion" or "composition". Right now we support composition only (all templates are equal from the platform point of view). Nesting and cache propagation would be hard to do in a scalable and performant manner in our current setup. It's not hard in general, it's just different from our current model. If there's limitations or other issues with that model, we can change it of course.

Jdlrobson added a comment.EditedDec 17 2019, 6:39 PM

@Krinkle that sounds sensible to me. Is there a way we can trigger a notice/log warning when partials are used?

Change 572933 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] WIP: Template should consider partials

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

This would mean that the place where the sub-template is rendered, an HTML (or text) parameter is used instead and the value set to to the expanded template. This could lead to better separation of concerns and better re-use

'Could' yes however my experience is that it doesn't, although I recognize it's hard to articulate that value. My experience with not using template partials is readability and maintainability. From experience template partials are much more pleasurable to work with then HTML strings when you are reusing template code. I've found less benefits where template partials exist purely to reduce large templates

For example using template partials here I consider bad:

<div>
{{>Header}}
</div>

whereas here it's useful:

<div>
{{#navigationHeader}}
{{>Header}}
{{/navigationHeader}}
</div>
<div>
{{#articleHeader}}
{{>Header}}
{{/articleHeader}}
</div>

While I cannot provide a full essay right now on why template partials are useful, I can provide some recent experiences in Vector.

Right now in Vector the template partial VectorMenu is used in two places - renderVariantsComponent and renderActionsComponent. Both of these menus show conditionally. Now if you were changing renderVariantsComponent you have to remember to check the renderActionsComponent. If you however reference VectorMenu inside the master template it becomes obvious right away that these can be used.

When template partials are used e.g. it's much clearer to see the 2 usages and how they might impact each other and there is less risk of regressions IMO.

Also when using template partials, you completely separate data from rendering inside your functions (you use a single entry point for rendering) and it becomes much clearer where further refactoring can occur e.g. renderVariantsComponent and renderNamespacesComponent in Vector the template data despite being identical uses different keys.

Jdlrobson updated the task description. (Show Details)Mon, Feb 24, 6:25 PM
ovasileva set the point value for this task to 8.Mon, Feb 24, 6:31 PM
ovasileva changed the point value for this task from 8 to 5.Mon, Feb 24, 7:12 PM

Change 574739 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/core@master] [WIP] TemplateParser: Make caching consider partials

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

phuedx added a subscriber: phuedx.Tue, Feb 25, 1:06 PM

@Jdlrobson Following on from our post-standup chat about this task and your patch, the above patch makes the TemplateParser keep track of the files being accessed during compilation and uses them to produce a hash which can then be checked when fetching a cached compilation result at a later date. This is very much the approach that @Krinkle suggested in T113095#2675078.

Even in the best case, the change increases the number of roundtrips to the FS and increases the amount of data read from the server-local object cache (APC or APCu) (and then deserialized). I suggest benchmarking the performance of the modified TemplateParser to better inform the discussion of the approach's merits.

Change 572933 abandoned by Jdlrobson:
Template should consider partials

Reason:
After talking to Roan and looking at Sam's alternative patch - https://gerrit.wikimedia.org/r/c/574739 seems to be preferable since it copies what we do for LESS in ResourceLoader.

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