API Exclude templates in 'themes' directories from $default theme (#5820)

API $themes parameter is now mandatory in TemplateLoader::findTemplate()

tests/core/manifest/TemplateLoaderTest.php

@@ -12,7 +12,15 @@ use SilverStripe\View\ThemeManifest;
 class TemplateLoaderTest extends SapphireTest {
 
 	private $base;
+
+	/**
+	 * @var ThemeManifest
+	 */
 	private $manifest;
+
+	/**
+	 * @var TemplateLoader
+	 */
 	private $loader;
 
 	/**
@@ -23,9 +31,9 @@ class TemplateLoaderTest extends SapphireTest {
 		// Fake project root
 		$this->base = dirname(__FILE__) . '/fixtures/templatemanifest';
 		// New ThemeManifest for that root
-		$this->manifest = new \SilverStripe\View\ThemeManifest($this->base, 'myproject', false, true);
+		$this->manifest = new ThemeManifest($this->base, 'myproject', false, true);
 		// New Loader for that root
-		$this->loader = new \SilverStripe\View\TemplateLoader($this->base);
+		$this->loader = new TemplateLoader($this->base);
 		$this->loader->addSet('$default', $this->manifest);
 	}
 
@@ -44,6 +52,28 @@ class TemplateLoaderTest extends SapphireTest {
 		);
 	}
 
+	public function testFindNestedThemeTemplates() {
+		// Without including the theme this template cannot be found
+		$this->assertEquals(null, $this->loader->findTemplate('NestedThemePage', ['$default']));
+
+		// With a nested theme available then it is available
+		$this->assertEquals(
+			"{$this->base}/module/themes/subtheme/templates/NestedThemePage.ss",
+			$this->loader->findTemplate('NestedThemePage', [
+				'silverstripe/module:subtheme',
+				'$default'
+			])
+		);
+
+		// Can also be found if excluding $default theme
+		$this->assertEquals(
+			"{$this->base}/module/themes/subtheme/templates/NestedThemePage.ss",
+			$this->loader->findTemplate('NestedThemePage', [
+				'silverstripe/module:subtheme',
+			])
+		);
+	}
+
 	/**
 	 * Test that 'main' and 'Layout' templates are loaded from set theme
 	 */

tests/core/manifest/fixtures/templatemanifest/module/themes/subtheme/templates/NestedThemePage.ss

@@ -0,0 +1 @@
+<p>Nested theme page</p>

tests/core/startup/ParameterConfirmationTokenTest.php

@@ -48,8 +48,10 @@ class ParameterConfirmationTokenTest extends SapphireTest {
 	}
 
 	public function tearDown() {
-		foreach($_GET as $param) {
-			if(stripos($param, 'parameterconfirmationtokentest_') === 0) unset($_GET[$param]);
+		foreach($_GET as $param => $value) {
+			if(stripos($param, 'parameterconfirmationtokentest_') === 0) {
+				unset($_GET[$param]);
+			}
 		}
 		$_SERVER['HTTP_HOST'] = $this->oldHost;
 		parent::tearDown();

view/TemplateLoader.php

@@ -17,6 +17,9 @@ class TemplateLoader {
 
 	protected $base;
 
+	/**
+	 * @var ThemeManifest[]
+	 */
 	protected $sets = [];
 
 	public static function instance() {
@@ -31,10 +34,30 @@ class TemplateLoader {
 		$this->base = $base ? $base : BASE_PATH;
 	}
 
+	/**
+	 * Add a new theme manifest for a given identifier. E.g. '$default'
+	 *
+	 * @param string $set
+	 * @param ThemeManifest $manifest
+	 */
 	public function addSet($set, $manifest) {
 		$this->sets[$set] = $manifest;
 	}
 
+	/**
+	 * Given a theme identifier, determine the path from the root directory
+	 *
+	 * The mapping from $identifier to path follows these rules:
+	 * - A simple theme name ('mytheme') which maps to the standard themes dir (/themes/mytheme)
+	 * - A theme path with a leading slash ('/mymodule/themes/mytheme') which maps directly to that path.
+	 * - or a vendored theme path. (vendor/mymodule:mytheme) which maps to the nested 'theme' within
+	 *   that module. ('/mymodule/themes/mytheme').
+	 * - A vendored module with no nested theme (vendor/mymodule) which maps to the root directory
+	 *   of that module. ('/mymodule').
+	 *
+	 * @param string $identifier Theme identifier.
+	 * @return string Path from root, not including leading forward slash. E.g. themes/mytheme
+	 */
 	public function getPath($identifier) {
 		$slashPos = strpos($identifier, '/');
 
@@ -44,6 +67,11 @@ class TemplateLoader {
 		}
 		// Otherwise if there is a "/", identifier is a vendor'ed module
 		elseif ($slashPos !== false) {
+			// Extract from <vendor>/<module>:<theme> format.
+			// <vendor> is optional, and if <theme> is omitted it defaults to the module root dir.
+			// If <theme> is included, this is the name of the directory under moduleroot/themes/
+			// which contains the theme.
+			// <module> is always the name of the install directory, not necessarily the composer name.
 			$parts = explode(':', $identifier, 2);
 
 			list($vendor, $module) = explode('/', $parts[0], 2);
@@ -75,22 +103,33 @@ class TemplateLoader {
 	 * format "type/name", where type is the type of template to search for
 	 * (e.g. Includes, Layout).
 	 *
-	 * @param  string|array $templates
-	 * @param  string $theme
-	 *
-	 * @return array
+	 * @param string|array $template Template name, or template spec in array format with the keys
+	 * 'type' (type string) and 'templates' (template hierarchy in order of precedence).
+	 * If 'templates' is ommitted then any other item in the array will be treated as the template
+	 * list.
+	 * Templates with an .ss extension will be treated as file paths, and will bypass
+	 * theme-coupled resolution.
+	 * @param array $themes List of themes to use to resolve themes. In most cases
+	 * you should pass in {@see SSViewer::get_themes()}
+	 * @return string Path to resolved template file, or null if not resolved.
 	 */
-	public function findTemplate($template, $themes = []) {
-
+	public function findTemplate($template, $themes) {
+		$type = '';
 		if(is_array($template)) {
-			$type = array_key_exists('type', $template) ? $template['type'] : '';
+			// Check if templates has type specified
+			if (array_key_exists('type', $template)) {
+				$type = $template['type'];
+				unset($template['type']);
+			}
+			// Templates are either nested in 'templates' or just the rest of the list
 			$templateList = array_key_exists('templates', $template) ? $template['templates'] : $template;
-		}
-		else {
-			$type = '';
+		} else {
 			$templateList = array($template);
 		}
 
+		// If we have an .ss extension, this is a path, not a template name. We should
+		// pass in templates without extensions in order for template manifest to find
+		// files dynamically.
 		if(count($templateList) == 1 && substr($templateList[0], -3) == '.ss') {
 			return $templateList[0];
 		}
@@ -113,6 +152,9 @@ class TemplateLoader {
 				}
 			}
 		}
+
+		// No template found
+		return null;
 	}
 
 }

view/ThemeManifest.php

@@ -98,6 +98,7 @@ class ThemeManifest {
 		$finder = new ManifestFileFinder();
 		$finder->setOptions(array(
 			'include_themes' => false,
+			'ignore_dirs' => array('node_modules', THEMES_DIR),
 			'ignore_tests'  => !$this->tests,
 			'dir_callback'  => array($this, 'handleDirectory')
 		));