Initial ideas to improve BlockViewTemplates.

BlockViewTemplate.php

<?php
defined('C5_EXECUTE') or die("Access Denied.");
/**
 * An object that represents a block's template, whether it's built-in, or custom.
 *
 * Summary of functional issues:
 * - Currently only custom templates can be overridden by packages, it's not possible for a 
 *   packaged theme to override root/blocks/<block_handle>/view.php.
 * - While packages can override custom templates, the search order is arbitrary.
 *
 * Functional Goals:
 * - Allow packages to override the default views as well as custom templates.
 * - Give the current theme (if it's in a package) the second highest priority (below root level 
 *   blocks dir) so it gets a chance to override templates from other packages
 *   PLEASE NOTE: the code below is already temporarily doing this with the new 
 *   method getPackagesInTemplateSearchPriority().
 *
 * Summary of implementation issues:
 * - The package search code is a bit weird and I think there's some logical errors.  See comments 
 *   below in the packages loop.
 * - There's heaps of roll your own paths that should be delegated to the things that know..
 *   E.g. The package search code could should use `$pkg->getPackagePath()` and should probably also 
 *   use something like `$pkg->getAssetPath`.
 * - The custom template if/else structure seems to be wrong.
 *
 * Implementation Goals:
 * - Improve the readability of the code so the algorithm is more visible at a conceptual level 
 *   i.e. search these locations for these styles.
 *   - get the code for handling the different template styles out of the main flow 
 *   (e.g. block_handle.php or block_handle/view.php)
 *   - create the notion of template search locations (root, package of the current theme, 
 *   other packages, core, block).
 * 
 * Proposal:
 * Still thinking about it but the idea would be to use Strategy+Delegate pattern. 
 * i.e. BlockViewTemplate would delegate to an actual template type/location to do the work.
 * - Introduce the notion of a BlockTemplateLocation (or BlockTemplateImpl) that BlockViewTemplate 
 *   delegates to.
 * - Create a BlockTemplateLocator class that finds all the current templates and returns 
 *   an array of them in priority order.  (i.e. <root>/blocks/, then current theme etc)
 * - BlockViewTemplate then just delegates to the highest priority BlockTemplateLocation.
 * - This would hopfully also allow a structured way for custom templates to wrap/decorate their parents. 
 * - Should probably also update Concrete5_Model_BlockType->getBlockTypeCustomTemplates() to use the same 
 *   locator code/logic (but finding only the custom templates).
 * 
 * 
 * Suggested usage:
 * public function computeView() {
 *   // return a list of template locations is priority order (index 0 is highest)
 *   // the getAll() method could be strategy based if needed for backward compatibility
 *   $this->templateLocations = BlockViewTemplateLocator::create($block, $customTemplateName)->getAll();
 * }
 * 
 * public function getPriortyLocation() {
 *   return $this->tempalteLocations[0]; // return the highest priorty template
 * }
 * 
 * Then BlockViewTemplate would delegate to the highest priority location.
 * 
 * public function getBasePath() { 
 *    return $this->getPriorityLocation()->getBasePath();
 * }
 * 
 * public function getTemplateHeaderItems() { 
 *    return $this->getPriorityLocation()->getTemplateHeaderItems();
 * }
 * 
 * 
 * Issues:
 * Backwards compatibility 
 * - New functionality may/will break of things.  Could be overcome by having the locator use 
 *   different strategies based on configuration.  
 *   E.g. define('BLOCK_VIEW_TEMPLATE_SEARCH_STRATEGY', 'Concrete_Library_Old_Block_Template_Search_Strategy');
 * - will things like http://andrewembler.com/posts/creating-a-simple-concrete5-wrapper-custom-block-template/ 
 *   continue to work?  Although using the stategy/delegate pattern it'd probably be pretty easy for a custom
 *   template to invoke the original with $template->getParentTemplate() or something similar.
 * 
 */
class BlockViewTemplate extends Concrete5_Library_BlockViewTemplate
{

    protected function computeView()
    {
        $bFilename = $this->bFilename;
        $obj = $this->obj;

        // if we've passed in "templates/" as the first part, we strip that off.
        if (strpos($bFilename, 'templates/') === 0) {
            $bFilename = substr($bFilename, 10);
        }

        // The filename might be a directory name with .php-appended (BlockView does that), strip it.
        $bFilenameWithoutDotPhp = $bFilename;
        if (substr($bFilename, -4) === ".php") {
            $bFilenameWithoutDotPhp = substr($bFilename, 0, strlen($bFilename) - 4);
        }

        // AP: we have a custom template... would be nice if this was if ($this->hasTemplate())
        if ($bFilename) {

            if (is_file(DIR_FILES_BLOCK_TYPES . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilename)) {

                // AP: <root>/blocks/<block_handle>/templates/bFilename.php
                $template = DIR_FILES_BLOCK_TYPES . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilename;
                $bv = new BlockView();
                $bv->setBlockObject($obj);
                $this->basePath = $bv->getBlockPath($this->render);
                $this->baseURL = $bv->getBlockURL();

            } else if (is_file(DIR_FILES_BLOCK_TYPES_CORE . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilename)) {

                // AP: <root>/concrete/blocks/<block_handle>/templates/bFilename.php
                $template = DIR_FILES_BLOCK_TYPES_CORE . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilename;
                $this->basePath = DIR_FILES_BLOCK_TYPES_CORE . '/' . $obj->getBlockTypeHandle();
                $this->baseURL = ASSETS_URL . '/' . DIRNAME_BLOCKS . '/' . $obj->getBlockTypeHandle();

            } else if (is_dir(DIR_FILES_BLOCK_TYPES . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilename)) {

                // AP: <root>/blocks/<block_handle>/templates/bFilename/view.php
                $template = DIR_FILES_BLOCK_TYPES . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilename . '/' . $this->render;
                $this->basePath = DIR_FILES_BLOCK_TYPES . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilename;
                $this->baseURL = DIR_REL . '/' . DIRNAME_BLOCKS . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilename;

            } else if (is_dir(DIR_FILES_BLOCK_TYPES_CORE . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilename)) {

                // AP: <root>/concrete/blocks/<block_handle>/templates/bFilename/view.php
                $template = DIR_FILES_BLOCK_TYPES_CORE . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilename . '/' . $this->render;
                $this->basePath = DIR_FILES_BLOCK_TYPES_CORE . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilename;
                $this->baseURL = ASSETS_URL . '/' . DIRNAME_BLOCKS . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilename;

                // AP: Not really sure what the scenario is here....
            } else if ($bFilename !== $bFilenameWithoutDotPhp) {


                if (is_dir(DIR_FILES_BLOCK_TYPES . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilenameWithoutDotPhp)) {

                    // AP: <root>/blocks/<block_handle>/templates/bFilenameWithoutDotPhp/view.php
                    $template = DIR_FILES_BLOCK_TYPES . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilenameWithoutDotPhp . '/' . $this->render;
                    $this->basePath = DIR_FILES_BLOCK_TYPES . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilenameWithoutDotPhp;
                    $this->baseURL = DIR_REL . '/' . DIRNAME_BLOCKS . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilenameWithoutDotPhp;

                } else if (is_dir(DIR_FILES_BLOCK_TYPES_CORE . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilenameWithoutDotPhp)) {

                    // AP: <root>/concrete/blocks/<block_handle>/templates/bFilenameWithoutDotPhp/view.php
                    $template = DIR_FILES_BLOCK_TYPES_CORE . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilenameWithoutDotPhp . '/' . $this->render;
                    $this->basePath = DIR_FILES_BLOCK_TYPES_CORE . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilenameWithoutDotPhp;
                    $this->baseURL = ASSETS_URL . '/' . DIRNAME_BLOCKS . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilenameWithoutDotPhp;
                }
            }

            // we check all installed packages
            if (!isset($template)) {

                // AP: I've changed this bit to put the theme last so it gets a chance to override it.
                // This is working well for my as now my theme package is happily being used in preference
                // to problogs default custom templates.
                $packages = $this->getPackagesInTemplateSearchPriority();

                foreach ($packages as $pkg) {
                    /** @var Package $pkg */
                    // AP: Getting the package root directory, why not use $pkg->getPackagePath() instead?
                    $d = '';
                    if (is_dir(DIR_PACKAGES . '/' . $pkg->getPackageHandle())) {
                        $d = DIR_PACKAGES . '/' . $pkg->getPackageHandle();
                    } else if (is_dir(DIR_PACKAGES_CORE . '/' . $pkg->getPackageHandle())) {
                        $d = DIR_PACKAGES_CORE . '/' . $pkg->getPackageHandle();
                    }

                    if ($d != '') {

                        // AP: if this isn't core we use the package directory otherwise we use core asset url (which also takes into account any updates)
                        // could we replace this with something like $pkg->getAssetPath()
                        $baseStub = (is_dir(DIR_PACKAGES . '/' . $pkg->getPackageHandle())) ? DIR_REL . '/' . DIRNAME_PACKAGES . '/' . $pkg->getPackageHandle() : ASSETS_URL . '/' . DIRNAME_PACKAGES . '/' . $pkg->getPackageHandle();

                        // AP: ARRRGGG!!!, now we're checking the package dirs but it could be either in core or not in core!!! Does this bugger
                        // the baseURL and basePath values we're getting here???
                        //
                        
                        if (is_file($d . '/' . DIRNAME_BLOCKS . '/' . $obj->getBlockTypeHandle() . '/' . $bFilename)) {

                            // AP: <root or core packages dir>/blocks/<block_handle>/bFilename.php
                            $template = $d . '/' . DIRNAME_BLOCKS . '/' . $obj->getBlockTypeHandle() . '/' . $bFilename;
                            $this->baseURL = ASSETS_URL . '/' . DIRNAME_BLOCKS . '/' . $obj->getBlockTypeHandle();
                            $this->basePath = DIR_FILES_BLOCK_TYPES_CORE . '/' . $obj->getBlockTypeHandle();
                        
                        } else if (is_file($d . '/' . DIRNAME_BLOCKS . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilename)) {

                            // AP: <root or core packages dir>/blocks/<block_handle>/templates/bFilename
                            $template = $d . '/' . DIRNAME_BLOCKS . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilename;
                            $this->baseURL = $baseStub . '/' . DIRNAME_BLOCKS . '/' . $obj->getBlockTypeHandle();
                            $this->basePath = $d . '/' . DIRNAME_BLOCKS . '/' . $obj->getBlockTypeHandle();
                        
                        } else if (is_dir($d . '/' . DIRNAME_BLOCKS . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilename)) {

                            // AP: <root or core packages dir>/blocks/<block_handle>/templates/bFilename/view.php
                            $template = $d . '/' . DIRNAME_BLOCKS . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilename . '/' . $this->render;
                            $this->baseURL = $baseStub . '/' . DIRNAME_BLOCKS . '/' . $obj->getBlockTypeHandle() . '/' . DIRNAME_BLOCK_TEMPLATES . '/' . $bFilename;

                        }
                    }

                    // AP: a continue at the end of a loop??? looks like a hang over from some refactoring.
                    // BTW: early on I thought this might be a mistake and should have been a break... but changing it to break broke things (c:
                    if ($this->baseURL != '') {
                        continue;
                    }
                }
            }

            // AP: We don't have a custom template so we look for overrides of the default views
            // first as a file, then as a directory.  I.e. we're searching in one place for two different styles.
        } else if (file_exists(DIR_FILES_BLOCK_TYPES . '/' . $obj->getBlockTypeHandle() . '.php')) {

            // AP: <root>/blocks/<block_handle>.php
            $template = DIR_FILES_BLOCK_TYPES . '/' . $obj->getBlockTypeHandle() . '.php';
            $bv = new BlockView();
            $bv->setBlockObject($obj);
            $this->baseURL = $bv->getBlockURL();
            $this->basePath = $bv->getBlockPath($this->render);

        } else if (file_exists(DIR_FILES_BLOCK_TYPES . '/' . $obj->getBlockTypeHandle() . '/' . $this->render)) {  // search <root>/blocks/<block_handle>/view.php

            // AP: <root>/blocks/<block_handle>/view.php
            $template = DIR_FILES_BLOCK_TYPES . '/' . $obj->getBlockTypeHandle() . '/' . $this->render;
            $this->baseURL = DIR_REL . '/' . DIRNAME_BLOCKS . '/' . $obj->getBlockTypeHandle();
        }

        // AP: Found no overrides so we use the blocks template.
        if (!isset($template)) {
            $bv = new BlockView();
            $bv->setBlockObject($obj);
            $template = $bv->getBlockPath($this->render) . '/' . $this->render;
            $this->baseURL = $bv->getBlockURL();
        }

        // AP: is this here because the template name + package search leaves this blank sometimes?
        if ($this->basePath == '') {
            $this->basePath = dirname($template);
        }

        $this->template = $template;
    }

    /**
     * Returns the packages in the order to search for template overrides.  By default this guarantees
     * to put the package of the current theme last (highest priority).
     * @return array
     */
    protected function getPackagesInTemplateSearchPriority()
    {
        $pl = PackageList::get();
        $packages = $pl->getPackages();
        $themePackageHandle = Page::getCurrentPage()->getCollectionThemeObject()->getPackageHandle();

        if ($themePackageHandle) {
            // the theme is in a package we pull it out it's position in the list and put it at the end.
            // This ensures it gets last say at providing template overrides.
            $temp = new Package();
            $themePackage = $temp->getByHandle($themePackageHandle);
            $packages = array_filter($packages, function ($pkg) use ($themePackage) {
                /** @var Package $pkg */
                return $pkg->getPackageID() != $themePackage->getPackageID();
            });
            $packages [] = $themePackage;
        }
        return $packages;
    }

}