Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API: Look for templates of namespaced classes in subfolders. #5490

Closed
wants to merge 3 commits into from

Conversation

sminnee
Copy link
Member

@sminnee sminnee commented May 9, 2016

This change will mean that SilverStripe\Control\Controller will look for its
template in templates/SilverStripe/Control/Controller.ss.

In order to preserve some backwards campatibility, non-namespaced classes
can have the templates stored in any template subfolder, but once you
add a namespace to a class, the namespaced path expression will need to
be a subfolder of /templates or themes//templates.

Layout and Content templates are stil supported as special template type,
Includes still functions but is a no-op. Other template subfolders should
not be used.

To do

  • Docs
  • Namespaces in includes

@dhensby
Copy link
Contributor

dhensby commented May 12, 2016

test failure :(

@sminnee
Copy link
Member Author

sminnee commented May 14, 2016

The test failure is caused because tests/controller/themes/controllertest/Controller.ss or tests/view/themes/layouttest/Controller.ss are being inadvertently included as unthemed templates for Controller, because themes aren't expected to be found in side tests/.*/themes/. I'll have a look at the previous behaviour and get something closer to it, if I can.

@sminnee
Copy link
Member Author

sminnee commented May 14, 2016

Needed some if(isset())s.

@sminnee
Copy link
Member Author

sminnee commented May 16, 2016

Needs docs, and the use of arbitrary folders should throw a deprecation notice

@wilr
Copy link
Member

wilr commented May 16, 2016

So if templates/SilverStripe/Control/Controller.ss has <% include Foo %> Foo.ss should not live in templates/Includes/?

SilverStripe in this case because the vendor is SilverStripe so if you install 3rdParty/silverstripe-bar then to override those templates they would be localled in templates/3rdParty/... If everything will just be in templates/SilverStripe it seems an unnecessary folder depth

Can we get the documentation updated alongside the PR?

@sminnee
Copy link
Member Author

sminnee commented May 16, 2016

So if templates/SilverStripe/Control/Controller.ss has <% include Foo %> Foo.ss should not live in templates/Includes/?

Oops, yeah... I hadn't thought that one through.

Probably the best best is to allow namespaces on includes, so we could <% include App\Regions\Foo %> and this would follow whatever namespace-to-folder-mapping was set up for App\Regions.

Using namespace semantics would be another option (so that Foo in the above example would resolve to SilverStripe\Control\Foo) but I think that will confuse more than help. Include statements aren't frequent enough for the verbosity to be a serious concern.

SilverStripe in this case because the vendor is SilverStripe so if you install 3rdParty/silverstripe-bar then to override those templates they would be localled in templates/3rdParty/... If everything will just be in templates/SilverStripe it seems an unnecessary folder depth

This PR matches PSR-0 folder conventions, which creates a lot of depth. But, yes, the SilverStripe namespace is only used for projects in the silverstripe/ composer vendor, if that's what you're getting at.

Building on this PR, I'd like to introduce more of a PSR-4 style resolver, where we can map subfolders to namespace prefixes. However, this simpler PR is intended to be the minimum necessary to let us start namespacing controller classes, so I don't want to bloat it.

Can we get the documentation updated alongside the PR?

Yeah, good call. At a minimum I'll put some terse docs into the PR.

@wilr
Copy link
Member

wilr commented May 16, 2016

@sminnee Include statements aren't frequent enough for the verbosity to be a serious concern. I'd be surprised if people aren't using includes as much as us (a certain telco provider has 145 in the includes and children folders) Things like displaying a users avatar with tooltip and edit link is reused across multiple different namespaces.

@sminnee
Copy link
Member Author

sminnee commented May 16, 2016

OK, fair point, but is specifying namespaces a problem, and if so, what would be a better approach? Lumping all includes into a single unnamespaced folder feels far from optimal.

We could have flags for selecting which namespaces to search, etc, but I'm worried about pre-emptively bloating out the number of special tags in the template system.

@sminnee
Copy link
Member Author

sminnee commented May 16, 2016

Ultimately, I guess you could do this, if we expected full namespaces:

templates/Includes/Something.ss

 <h1>Something</1h> 

templates/App/PageType/Page.ss:

<% include Includes/Something %>

OK for a first-cut?

@axyr
Copy link
Contributor

axyr commented May 17, 2016

I think Also that Includes are heavily used. At least we do as well..

Sam Minnee added 2 commits May 21, 2016 16:02
This change will mean that SilverStripe\Control\Controller will look for its
template in templates/SilverStripe/Control/Controller.ss.

In order to preserve some backwards campatibility, non-namespaced classes
can have the templates stored in any template subfolder, but once you
add a namespace to a class, the namespaced path expression will need to
be a subfolder of <module>/templates or themes/<theme>/templates.

Layout and Content templates are stil supported as special template type,
Includes still functions but is a no-op. Other template subfolders should
not be used.
@sminnee
Copy link
Member Author

sminnee commented May 21, 2016

OK, I've made those changes now. Ready for merge? I'd like to get this in so that we can start namespacing core classes that have templates attached.

@tractorcow
Copy link
Contributor

I've tested this out locally, and am getting some really weird behaviour.

image

does it need a rebase maybe?

@tractorcow
Copy link
Contributor

Rebasing fixes it. :D

@tractorcow
Copy link
Contributor

Merged with 19646d1

@tractorcow tractorcow closed this Jun 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants