Clarify `Includes/` and template path use in SS4

[chillu]

We need some clarity around template references in PHP and SS files, in particular how strong the relationship between folder and file is. At the moment there's ambiguity between forward slash (=folder separator) or back slack (=namespace separator). Template references can be a composite between namespaces and folder paths.

Example:

MyFile.ss
Includes/
  Myfile.ss
MyNamespace/
  MyFile.ss
  Includes/
    MyFile.ss

• SSViewer chooses MyNamespace/MyFile.ss when passed the MyNamespace/MyFile PHP class (higher priority)
• <% include MyNamespace/MyFile %> chooses MyNamespace/Includes/MyFile.ss
• <% include MyNamespace/Includes/MyFile %> chooses MyNamespace/Includes/MyFile.ss
• <% include MyFile %> chooses Includes/MyFile.ss
• <% include Includes/MyNamespace/MyFile %> doesn't work
• <% include Includes/MyFile %> doesn't work

Here's the status quo:

• SS3 has template inheritance based on class inheritance
• SS3 has template "types": Subfolders which contain templates accessible via SSViewer::getTemplateFileByType()
• The most commonly used "type" is Includes. Templates in this path are included by default in SSViewer::get_templates_by_class() (with lower priority than templates without a "type").
• SS3 has special template placeholders for some "types": Templates in Layout/ and Content/ subfolders can be used as $Layout and $Content placeholders
• SS4 introduces namespaces to templates, matching them to folder paths
• SS4 supports forward slashes, back slashes and escaped back slashes for template references in *.ss
• SS4 finds namespaced templates by "type" if its the innermost folder (e.g. MyNamespace/Includes/MyFile.ss, see ThemeResourceLoader@findTemplate)

The docs mention:

Note that in SilverStripe 3, you didn't have to specify a namespace in your include tag, as the template engine didn't use namespaces. As of SilverStripe 4, the template namespaces need to match the folder structure of your template files.

They're ambiguous about how to combine "type" and namespace folders:

When the class has a namespace, the namespace will be interpreted as subfolder within the templates path. For, example, the class SilverStripe\Control\Controller will be rendered with the templates/SilverStripe/Control/Controller.ss template.

Sam recommended removing the Includes/ folder (BC only) in SS4: https://github.com/silverstripe/silverstripe-framework/issues/5445

What is the Includes folder, anyway? With all these new folders to support namespaces, the Includes folder make less sense. Although Layout and Content have special meaning for the template renderer, Includes does not. I recommend we remove this convention and any special supporting code for it.

The theme stacking/cascading PR only mentions the following in the upgrading guides, without changing any other docs (see changelog):

Make sure templates are in correct locations. Templates are now much more strict about their locations. You can no longer put a template in an arbitrary folder and have it be found. Case is now also checked on case-sensitive filesystems. Either include the folder in the template name (renderWith('Field.ss') => renderWith('forms/Field.ss')), move the template into the correct directory, or both.

[chillu]

@sminnee's take (pasted from Slack):

I think that the whole Includes folder should probably be treated more as a BC thing than a recommended feature. So <% incude A\B %> should work with templates/A/B.ss if it exists, and use templates/Includes/A/B.ss only as a failover

And regarding \ vs /: blackslash should function without escaping as it’s about class namespaces. “SilverStripe\Security\LoginForm” is a canonical template name, and it will be looked for at the moment in templates/SilverStripe/Security/LoginForm.ss. The model to compare it to is PSR autoloaders (in this case PSR-0)

We may at some stage implement a PSR-4 analogue, and then the mapping between template namespaces and filenames will become a little less direct. Using “/“ would therefore send the wrong message.

For includes, we haven’t yet put much thought into how we recommend that includes are namespaced in SS4 projects. Probably we’d recommend similar conventions as PHP classes. so SilverStripe\Blog\PageTypes\BlogHolder might include SilverStripe\Blog\BlogList or SilverStripe\Blog\Components\BlogList, or even SilverStripe\Blog\Includes\BlogList

Sam created an RFC: Template filename selection for namespaced contollers which might be relevant here, since it introduces the concept of template namespace aliases. framework/templates/core/Controller.ss has core aliased to the SilverStripe\Controller namespace.

@tractorcow's take:

templates have always been path based, so trying to shoe horn it into a php class namespaced analogy would be misleading also

[sminnee]

What do people think about treating any include reference that starts with a "." as being relative to the caller template

So if you have <% include ./subfolder/SubTemplate %> in templates/App/PageTypes/Page.ss, it would include templates/App/PageTypes/subfolder/SubTemplate.ss.

As you might expect <% include ../siblingfolder/SubTemplate %> would also work.

Anything else will be a global fully qualified template name, using the current logic.

(Inspiration for this comes from the node require() semantics)

[chillu]

Discussed this a bit further with Sam offline, and this could get horribly confusing if you factor in the template cascade: Are paths relative to the "template root" where the including template is located, or does the cascade kick in? It's overcomplicating an already complicated system.

[chillu]

Discussed a bit more with @tractorcow - result:

• Add multiple 'type' support via arrays in SSViewer@findTemplate
• Support an "empty" type
• Change SSTemplateParser to use this, looking for empty type first, then for type=Include (SSViewer::execute_template(["type" => ["", "Includes"], '.$template.']...). This maintains BC.
• Change SSViewer@get_templates_by_class to list templates for all types passed into the method
• Mark implicit use of Includes/ (<% include MyFile %>) as deprecated, throw warning
• Fix core module <% include %> use
• Update changelog and docs to reflect this

[kinglozzer]

And regarding \ vs /: blackslash should function without escaping as it’s about class namespaces. “SilverStripe\Security\LoginForm” is a canonical template name, and it will be looked for at the moment in templates/SilverStripe/Security/LoginForm.ss

My preference would be for every variation of it to work, and include the same template (in order of most-least important, if we can’t have them all for whatever reason):

• <% include SilverStripe/Blog/Components/BlogList %>
• <% include SilverStripe\Blog\Components\BlogList %>
• <% include SilverStripe\\Blog\\Components\\BlogList %>

Is there any technical reason we couldn’t achieve this?

I think that the whole Includes folder should probably be treated more as a BC thing than a recommended feature. So <% incude A\B %> should work with templates/A/B.ss if it exists, and use templates/Includes/A/B.ss only as a failover

I think this is acceptable - we’re not removing the <% include %> tag, just the special treatment for the “Includes/” folder name. So <% include B %> would work with templates/Includes/B.ss in SilverStripe 4 (unless templates/B.ss is present), but you’d need to do <% include Includes/B %> in SilverStripe 5.

What do people think about treating any include reference that starts with a "." as being relative to the caller template.

Looks like you’ve already discussed this anyway, but I wasn’t sure what the benefit of this approach would be for templates. I think it’d be more likely to introduce headaches: if you move a template up or down a level, or to a different theme, your paths would be wrong - something that the extra verbosity of the current approach avoids.

[tractorcow]

I'm not sure I'm happy going down this path; The existing convention we have in place whereby all <% includes %> look up files in an Include/ path (similar to how $Layout looks up templates in a Layout/ path) means that we can be certain that any time we are using an Include, we won't end up including a similarly-named template which wasn't intended to be included. It provides a level of structure and segmentation to templates.

And regarding \ vs /: blackslash should function without escaping as it’s about class namespaces. “SilverStripe\Security\LoginForm” is a canonical template name, and it will be looked for at the moment in templates/SilverStripe/Security/LoginForm.ss

It's really about file paths, because includes aren't backed by classes, they are template only. I prefer to retain / as the standard filesystem separator, but support any slashes.

If we were to dump the Include/ prefix, then the only time "type" really has an effect anymore is on $Layout / $Content. Given they are pretty important there (where we COULD have non-Layout templates of the same name which MUST not be treated as Layout) I disagree that we could ever really move away from the whole concept of having a template "type". Yet this seems like we are watering it down by saying Includes don't need to have a type anymore.

Supporting "both" by saying Includes will look in multiple places essentially immediately removes all benefits of being able to segment your templates anyway. I'm not in favour of ambiguous file paths for template lookups anyway (psr-4 isn't ambiguous either).

[tractorcow]

Ok, so here's some counterarguments to my own argument.

Some templates (e.g. Form.ss) are used BOTH as <% include %> and as normal regular templates. In this case it NEEDS to be in the Include dir, even if it's not used as an include.

E.g. see ModelAdmin_EditForm.ss

[sminnee]

The point is that $this->renderWith($x) should pick up the same template as <% include x %>

As a short-term hack (which seems to cause a dismaying amount of confusion) <% include SubTemplate %> would also pick up templates/Includes/SubTemplate.ss, simply to aid backward compatibility.

If we were to dump the Include/ prefix, then the only time "type" really has an effect anymore is on $Layout / $Content.

Exactly.

Yet this seems like we are watering it down by saying Includes don't need to have a type anymore.

Is it easier to stop thinking of "Includes" as a type, then? Ingo's suggested implementation co-opts the use of the type system to provides a backward-compatibility service. That would still be allowed.

[sminnee]

My preference would be for every variation of it to work, and include the same template (in order of most-least important, if we can’t have them all for whatever reason): <% include SilverStripe/Blog/Components/BlogList %>, <% include SilverStripe\Blog\Components\BlogList %>, <% include SilverStripe\\Blog\\Components\\BlogList %>

That seems fine to me, but our examples should always use the first style.

[chillu]

The existing convention we have in place whereby all <% includes %> look up files in an Include/ path (similar to how $Layout looks up templates in a Layout/ path) means that we can be certain that any time we are using an Include, we won't end up including a similarly-named template which wasn't intended to be included. It provides a level of structure and segmentation to templates.

In 3.x, <% include %> still referenced the end of a file path, just leaving out Includes/. In 4.x, you have this namespaced weirdness: <% include MyNamespace/MyFile %> chooses MyNamespace/Includes/MyFile.ss. MyNamespace/MyFile is no longer the end of a file path, the Includes/ is wedged in there. So you're disassociating template identifiers from file paths, in my opinion that provides less structure than you gain from the 3.x "prepend Includes" convention. And you never have to write out those file paths for $Content and $Layout yourself, hence the stance of "watering down types" doesn't hold water for me.

[tractorcow]

Ok sure, so we support type (as before) for Layout / Content, but drop it for Includes... that's fine with me, after some thinking about it.

[dhensby]

A little late to the party here:

My preference would be for every variation of it to work, and include the same template (in order of most-least important, if we can’t have them all for whatever reason):

I don't like this, I'd rather there was 1 right way to do it rather then 3 wrong ways. It's extra dev work, it's extra maintenance and when we inevitably conclude in a few years this was silly, we'll break a load of modules/themes/templates because we gave too many options to devs.

I'm not sure I'm happy going down this path; The existing convention we have in place whereby all <% includes %> look up files in an Include/ path (similar to how $Layout looks up templates in a Layout/ path) means that we can be certain that any time we are using an Include, we won't end up including a similarly-named template which wasn't intended to be included. It provides a level of structure and segmentation to templates.

I quite like this idea, actually - forcing include templates into the includes dir does seem advantageous in terms of reducing assumptions, template look-ups and unexpected behaviour. I don't think this stops us using $this->renderWith($x) and having it behave the same way?

The type of slash we use to separate the template "namespaces" is not an important point for debate, IMO. Just pick one and do it. Also remember that \ is a path separator in windows so it's not necessarily explicitly a namespace separator.

One question I have is how does one override a namespaced template (eg: an include) that comes with a module in our own theme/module?

[tractorcow]

I quite like this idea, actually - forcing include templates into the includes dir does seem advantageous in terms of reducing assumptions, template look-ups and unexpected behaviour. I don't think this stops us using $this->renderWith($x) and having it behave the same way?

I think the downside I found out about this is that, if any template may EVER need to be included via <% include %> , then it needs to be in the Includes dir. The result is that just about everything is in that dir, making it redundant.

[tractorcow]

The type of slash we use to separate the template "namespaces" is not an important point for debate, IMO. Just pick one and do it. Also remember that \ is a path separator in windows so it's not necessarily explicitly a namespace separator.

/ doesn't need to be escaped :D

[dhensby]

I think the downside I found out about this is that, if any template may EVER need to be included via <% include %> , then it needs to be in the Includes dir. The result is that just about everything is in that dir, making it redundant.

good point

/ doesn't need to be escaped :D

/ it is

[chillu]

I think the Includes/ folder should be a "soft convention": Developers can choose to use it if it helps their conceptual model for how templates are structured, on the same level of choosing to place templates in Elements/ or Components/. But with the introduction of namespaces, it's actively confusing devs to enforce this convention (template identifier != file path).

[tractorcow]

Yes, we can support it the way you suggested with the code sample earlier @chillu ; Support standard template paths, but also check "type" => "Includes" as a fallback.