From 4b4aa4e91c31b930e664527994840895fe176466 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Tue, 19 Jul 2016 13:17:07 +1200 Subject: [PATCH] API Exclude templates in 'themes' directories from $default theme (#5820) API $themes parameter is now mandatory in TemplateLoader::findTemplate() --- tests/core/manifest/TemplateLoaderTest.php | 34 +++++++++- .../subtheme/templates/NestedThemePage.ss | 1 + .../ParameterConfirmationTokenTest.php | 6 +- view/TemplateLoader.php | 62 ++++++++++++++++--- view/ThemeManifest.php | 1 + 5 files changed, 90 insertions(+), 14 deletions(-) create mode 100644 tests/core/manifest/fixtures/templatemanifest/module/themes/subtheme/templates/NestedThemePage.ss diff --git a/tests/core/manifest/TemplateLoaderTest.php b/tests/core/manifest/TemplateLoaderTest.php index ebcb7cb0b..f41845fd7 100644 --- a/tests/core/manifest/TemplateLoaderTest.php +++ b/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 */ diff --git a/tests/core/manifest/fixtures/templatemanifest/module/themes/subtheme/templates/NestedThemePage.ss b/tests/core/manifest/fixtures/templatemanifest/module/themes/subtheme/templates/NestedThemePage.ss new file mode 100644 index 000000000..68716bf0b --- /dev/null +++ b/tests/core/manifest/fixtures/templatemanifest/module/themes/subtheme/templates/NestedThemePage.ss @@ -0,0 +1 @@ +

Nested theme page

diff --git a/tests/core/startup/ParameterConfirmationTokenTest.php b/tests/core/startup/ParameterConfirmationTokenTest.php index 942821dcb..fac5caf20 100644 --- a/tests/core/startup/ParameterConfirmationTokenTest.php +++ b/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(); diff --git a/view/TemplateLoader.php b/view/TemplateLoader.php index 8a7a58695..5c66e1d8a 100644 --- a/view/TemplateLoader.php +++ b/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 /: format. + // is optional, and if is omitted it defaults to the module root dir. + // If is included, this is the name of the directory under moduleroot/themes/ + // which contains the theme. + // 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; } } diff --git a/view/ThemeManifest.php b/view/ThemeManifest.php index 1e17bedf0..06f04fdb8 100644 --- a/view/ThemeManifest.php +++ b/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') ));