BUG Fix sub-template lookup for includes

docs/en/04_Changelogs/4.0.0.md

@@ -187,17 +187,23 @@ Templates are now much more strict about their locations. You can no longer put
 folder and have it be found. Case is now also checked on case-sensitive filesystems.
 
 Either include the folder in the template name (`renderWith('MyEmail.ss')` => `renderWith('emails/MyEmail.ss')`),
-move the template into the correct directory, or both. This also affects `<% include %>` statements in templates.
-
-When using `<% include %>` template tag you should continue to leave out the `Includes` folder.
-`<% include Sidebar %>` will match only match `Includes/Sidebar.ss`, not `Sidebar.ss`.
-Please refer to our [template syntax](/developer_guides/templates/syntax) for details.
+move the template into the correct directory, or both.
 
 Core template locations have moved - if you're including or overriding these
 (e.g. for FormField templates) please adjust to the new paths. The `forms` folder
 no longer exists, and instead template locations will be placed in paths that match
 the `SilverStripe\Forms` namespace.
 
+#### Upgrade <% include %>
+
+When using `<% include %>` template tag you can continue to leave out the `Includes` folder,
+but this now will also search templates in the base folder if no Include can be found.
+
+`<% include Sidebar %>` will match `Includes/Sidebar.ss`, but will also match `Sidebar.ss`
+if the former is not present.
+
+Please refer to our [template syntax](/developer_guides/templates/syntax) for details.
+
 #### Upgrade static config settings to `private static`
 
 If you have some class configuration statics defined and they aren't private, 

src/View/SSTemplateParser.peg

@@ -891,6 +891,7 @@ class SSTemplateParser extends Parser implements TemplateParser
         $template = $res['template'];
         $arguments = $res['arguments'];
 
+        // Note: 'type' here is important to disable subTemplates in SSViewer::getSubtemplateFor()
         $res['php'] = '$val .= \\SilverStripe\\View\\SSViewer::execute_template([["type" => "Includes", '.$template.'], '.$template.'], $scope->getItem(), array(' .
             implode(',', $arguments)."), \$scope);\n";
 

src/View/SSTemplateParser.php

@@ -3499,6 +3499,7 @@ class SSTemplateParser extends Parser implements TemplateParser
         $template = $res['template'];
         $arguments = $res['arguments'];
 
+        // Note: 'type' here is important to disable subTemplates in SSViewer::getSubtemplateFor()
         $res['php'] = '$val .= \\SilverStripe\\View\\SSViewer::execute_template([["type" => "Includes", '.$template.'], '.$template.'], $scope->getItem(), array(' .
             implode(',', $arguments)."), \$scope);\n";
 

src/View/SSViewer.php

@@ -474,6 +474,8 @@ class SSViewer implements Flushable
         $scope = new SSViewer_DataPresenter($item, $overlay, $underlay, $inheritedScope);
         $val = '';
 
+        // Placeholder for values exposed to $cacheFile
+        [$cache, $scope, $val];
         include($cacheFile);
 
         return $val;
@@ -519,26 +521,26 @@ class SSViewer implements Flushable
         // Makes the rendered sub-templates available on the parent item,
         // through $Content and $Layout placeholders.
         foreach (array('Content', 'Layout') as $subtemplate) {
-            $sub = null;
-            if (isset($this->subTemplates[$subtemplate])) {
-                $sub = $this->subTemplates[$subtemplate];
-            } elseif (!is_array($this->templates)) {
-                $sub = ['type' => $subtemplate, $this->templates];
-            } elseif (!array_key_exists('type', $this->templates) || !$this->templates['type']) {
-                $sub = array_merge($this->templates, ['type' => $subtemplate]);
+            // Detect sub-template to use
+            $sub = $this->getSubtemplateFor($subtemplate);
+            if (!$sub) {
+                continue;
             }
 
-            if ($sub) {
+            // Create lazy-evaluated underlay for this subtemplate
+            $underlay[$subtemplate] = function () use ($item, $arguments, $sub) {
                 $subtemplateViewer = clone $this;
                 // Disable requirements - this will be handled by the parent template
                 $subtemplateViewer->includeRequirements(false);
                 // Select the right template
                 $subtemplateViewer->setTemplate($sub);
 
+                // Render if available
                 if ($subtemplateViewer->exists()) {
-                    $underlay[$subtemplate] = $subtemplateViewer->process($item, $arguments);
-                }
+                    return $subtemplateViewer->process($item, $arguments);
                 }
+                return null;
+            };
         }
 
         $output = $this->includeGeneratedTemplate($cacheFile, $item, $arguments, $underlay, $inheritedScope);
@@ -564,7 +566,44 @@ class SSViewer implements Flushable
             }
         }
 
-        return DBField::create_field('HTMLFragment', $output);
+        /** @var DBHTMLText $html */
+        $html = DBField::create_field('HTMLFragment', $output);
+        return $html;
+    }
+
+    /**
+     * Get the appropriate template to use for the named sub-template, or null if none are appropriate
+     *
+     * @param string $subtemplate Sub-template to use
+     *
+     * @return array|null
+     */
+    protected function getSubtemplateFor($subtemplate)
+    {
+        // Get explicit subtemplate name
+        if (isset($this->subTemplates[$subtemplate])) {
+            return $this->subTemplates[$subtemplate];
+        }
+
+        // Don't apply sub-templates if type is already specified (e.g. 'Includes')
+        if (isset($this->templates['type'])) {
+            return null;
+        }
+
+        // Filter out any other typed templates as we can only add, not change type
+        $templates = array_filter(
+            (array)$this->templates,
+            function ($template) {
+                return !isset($template['type']);
+            }
+        );
+        if (empty($templates)) {
+            return null;
+        }
+
+        // Set type to subtemplate
+        $templates['type'] = $subtemplate;
+        return $templates;
     }
 
     /**

src/View/SSViewer_DataPresenter.php

@@ -4,7 +4,7 @@ namespace SilverStripe\View;
 
 use InvalidArgumentException;
 use SilverStripe\Core\ClassInfo;
-use SilverStripe\Core\Injector\Injector;
+use SilverStripe\ORM\FieldType\DBField;
 
 /**
  * This extends SSViewer_Scope to mix in data on top of what the item provides. This can be "global"
@@ -15,12 +15,25 @@ use SilverStripe\Core\Injector\Injector;
  */
 class SSViewer_DataPresenter extends SSViewer_Scope
 {
-
+    /**
+     * List of global property providers
+     *
+     * @internal
+     * @var TemplateGlobalProvider[]|null
+     */
     private static $globalProperties = null;
+
+    /**
+     * List of global iterator providers
+     *
+     * @internal
+     * @var TemplateIteratorProvider[]|null
+     */
     private static $iteratorProperties = null;
 
     /**
      * Overlay variables. Take precedence over anything from the current scope
+     *
      * @var array|null
      */
     protected $overlay;
@@ -117,86 +130,30 @@ class SSViewer_DataPresenter extends SSViewer_Scope
      */
     public function getInjectedValue($property, $params, $cast = true)
     {
-        $on = $this->itemIterator ? $this->itemIterator->current() : $this->item;
-
-        // Find the source of the value
-        $source = null;
-
-        // Check for a presenter-specific override
-        if (array_key_exists($property, $this->overlay)) {
-            $source = array('value' => $this->overlay[$property]);
-        } // Check if the method to-be-called exists on the target object - if so, don't check any further
-        // injection locations
-        else {
-            if (isset($on->$property) || method_exists($on, $property)) {
-                $source = null;
-            } // Check for a presenter-specific override
-            else {
-                if (array_key_exists($property, $this->underlay)) {
-                    $source = array('value' => $this->underlay[$property]);
-                } // Then for iterator-specific overrides
-                else {
-                    if (array_key_exists($property, self::$iteratorProperties)) {
-                        $source = self::$iteratorProperties[$property];
-                        if ($this->itemIterator) {
-                            // Set the current iterator position and total (the object instance is the first item in
-                            // the callable array)
-                            $source['implementer']->iteratorProperties(
-                                $this->itemIterator->key(),
-                                $this->itemIteratorTotal
-                            );
-                        } else {
-                            // If we don't actually have an iterator at the moment, act like a list of length 1
-                            $source['implementer']->iteratorProperties(0, 1);
+        // Get source for this value
+        $source = $this->getValueSource($property);
+        if (!$source) {
+            return null;
         }
-                    } // And finally for global overrides
-                    else {
-                        if (array_key_exists($property, self::$globalProperties)) {
-                            $source = self::$globalProperties[$property];  //get the method call
-                        }
-                    }
-                }
-            }
-        }
-
-        if ($source) {
-            $res = array();
 
         // Look up the value - either from a callable, or from a directly provided value
+        $res = [];
         if (isset($source['callable'])) {
             $res['value'] = call_user_func_array($source['callable'], $params);
         } elseif (isset($source['value'])) {
             $res['value'] = $source['value'];
         } else {
-                throw new InvalidArgumentException("Injected property $property does't have a value or callable " .
-                    "value source provided");
+            throw new InvalidArgumentException(
+                "Injected property $property does't have a value or callable value source provided"
+            );
         }
 
         // If we want to provide a casted object, look up what type object to use
         if ($cast) {
-                // If the handler returns an object, then we don't need to cast.
-                if (is_object($res['value'])) {
-                    $res['obj'] = $res['value'];
-                } else {
-                    // Get the object to cast as
-                    $casting = isset($source['casting']) ? $source['casting'] : null;
-
-                    // If not provided, use default
-                    if (!$casting) {
-                        $casting = ViewableData::config()->uninherited('default_cast');
+            $res['obj'] = $this->castValue($res['value'], $source);
         }
-
-                    $obj = Injector::inst()->get($casting, false, array($property));
-                    $obj->setValue($res['value']);
-
-                    $res['obj'] = $obj;
-                }
-            }
-
         return $res;
     }
-        return null;
-    }
 
     /**
      * Store the current overlay (as it doesn't directly apply to the new scope
@@ -204,6 +161,7 @@ class SSViewer_DataPresenter extends SSViewer_Scope
      * "up" in the stack (hence upIndex), rather than the current item, because
      * SSViewer_Scope::obj() has already been called and pushed the new item to
      * the stack by this point
+     *
      * @return SSViewer_Scope
      */
     public function pushScope()
@@ -226,6 +184,7 @@ class SSViewer_DataPresenter extends SSViewer_Scope
      * Now that we're going to jump up an item in the item stack, we need to
      * restore the overlay that was previously stored against the next item "up"
      * in the stack from the current one
+     *
      * @return SSViewer_Scope
      */
     public function popScope()
@@ -275,7 +234,8 @@ class SSViewer_DataPresenter extends SSViewer_Scope
             }
         }
 
-        return parent::obj($name, $arguments, $cache, $cacheName);
+        parent::obj($name, $arguments, $cache, $cacheName);
+        return $this;
     }
 
     public function getObj($name, $arguments = [], $cache = false, $cacheName = null)
@@ -316,4 +276,110 @@ class SSViewer_DataPresenter extends SSViewer_Scope
             return parent::__call($name, $arguments);
         }
     }
+
+    /**
+     * Evaluate a template override
+     *
+     * @param string $property Name of override requested
+     * @param array $overrides List of overrides available
+     * @return null|array Null if not provided, or array with 'value' or 'callable' key
+     */
+    protected function processTemplateOverride($property, $overrides)
+    {
+        if (!isset($overrides[$property])) {
+            return null;
+        }
+
+        // Detect override type
+        $override = $overrides[$property];
+
+        // Late-evaluate this value
+        if (is_callable($override)) {
+            $override = $override();
+
+            // Late override may yet return null
+            if (!isset($override)) {
+                return null;
+            }
+        }
+
+        return [ 'value' => $override ];
+    }
+
+    /**
+     * Determine source to use for getInjectedValue
+     *
+     * @param string $property
+     * @return array|null
+     */
+    protected function getValueSource($property)
+    {
+        // Check for a presenter-specific override
+        $overlay = $this->processTemplateOverride($property, $this->overlay);
+        if (isset($overlay)) {
+            return $overlay;
+        }
+
+        // Check if the method to-be-called exists on the target object - if so, don't check any further
+        // injection locations
+        $on = $this->itemIterator ? $this->itemIterator->current() : $this->item;
+        if (isset($on->$property) || method_exists($on, $property)) {
+            return null;
+        }
+
+        // Check for a presenter-specific override
+        $underlay = $this->processTemplateOverride($property, $this->underlay);
+        if (isset($underlay)) {
+            return $underlay;
+        }
+
+        // Then for iterator-specific overrides
+        if (array_key_exists($property, self::$iteratorProperties)) {
+            $source = self::$iteratorProperties[$property];
+            /** @var TemplateIteratorProvider $implementer */
+            $implementer = $source['implementer'];
+            if ($this->itemIterator) {
+                // Set the current iterator position and total (the object instance is the first item in
+                // the callable array)
+                $implementer->iteratorProperties(
+                    $this->itemIterator->key(),
+                    $this->itemIteratorTotal
+                );
+            } else {
+                // If we don't actually have an iterator at the moment, act like a list of length 1
+                $implementer->iteratorProperties(0, 1);
+            }
+            return $source;
+        }
+
+        // And finally for global overrides
+        if (array_key_exists($property, self::$globalProperties)) {
+            return self::$globalProperties[$property];  //get the method call
+        }
+
+        // No value
+        return null;
+    }
+
+    /**
+     * Ensure the value is cast safely
+     *
+     * @param mixed $value
+     * @param array $source
+     * @return DBField
+     */
+    protected function castValue($value, $source)
+    {
+        // Already cast
+        if (is_object($value)) {
+            return $value;
+        }
+
+        // Get provided or default cast
+        $casting = empty($source['casting'])
+            ? ViewableData::config()->uninherited('default_cast')
+            : $source['casting'];
+
+        return DBField::create_field($casting, $value);
+    }
 }