Sage: Custom templates not loading since latest update

[vdrnn]

Submit a feature request or bug report

• [x] I've read the guidelines for Contributing to Roots Projects
• [ ] This is a feature request
• [x] This is a bug report
• [x] This request isn't a duplicate of an existing issue
• [x] I've read the docs and NPM Debugging Guidelines post and followed them (if applicable)
• [x] This is not a personal support request that should be posted on the Roots Discourse forums

Bug report

It looks like after the latest update of sage, the custom templates aren't working anymore. I tested it on two different installations and for both the custom templates don't appear under page attributes. I guess it has something to do with the move of templates/ to resources/views/ and src/ to app/ but unfortunately I wasn't able to figure out where the problem is.

Specifically I'm talking about this template/templates resources/views/template-custom.blade.php

[retlehs]

confirmed bug, thanks for reporting

[retlehs]

it looks like wordpress hardcodes looking for page templates beyond 1 level deep 😱, see http://wordpress.stackexchange.com/a/250024 / https://github.com/WordPress/WordPress/blob/503d42fe6bcd2f45b3af78657686f8a27ccb0136/wp-includes/class-wp-theme.php#L1034-L1045

from what i can tell so far, we'd have to duplicate the get_post_templates functionality and tell it to search beyond 1 folder deep

...then use the theme_page_templates filter to populate the list of template names & filenames

[Log1x]

oh jeeze

[derkjn]

How about setting the templates in the cache to avoid the issue entirely? Since that method is checking with $post_templates = $this->cache_get( 'post_templates' );?

[retlehs]

@derkjn how would the issue be avoided by doing that? that solution still requires duplicating a bunch of functionality in the WP theme class

[derkjn]

If we preset the list of templates in the cache before wordpress gets to that point, instead of searching for php files in the first level deep, it will return what's in the cache, which basically goes down to accessing the following:

function wp_cache_add( $key, $data, $group = '', $expire = 0 ) {
    global $wp_object_cache;

    return $wp_object_cache->add( $key, $data, $group, (int) $expire );
}

[retlehs]

right. where are you getting the list of custom page templates from without duplicating the functionality from the WP theme class?

[derkjn]

The only thing you need to duplicate is the regexp to parse the files, other than that why not just scandir the templates folder?
Basically you only need the following to feed the cache:

$files = scandir(get_template_directory_uri());
    foreach ( $files as $file => $full_path ) {
        if ( ! preg_match( '|Template Name:(.*)$|mi', file_get_contents( $full_path ), $header ) ) {
            continue;
        }

        $types = array( 'page' );
        if ( preg_match( '|Template Post Type:(.*)$|mi', file_get_contents( $full_path ), $type ) ) {
            $types = explode( ',', _cleanup_header_comment( $type[1] ) );
        }

        foreach ( $types as $type ) {
            $type = sanitize_key( $type );
            if ( ! isset( $post_templates[ $type ] ) ) {
                $post_templates[ $type ] = array();
            }

            $post_templates[ $type ][ $file ] = _cleanup_header_comment( $header[1] );
        }
    }

[retlehs]

i mean we're not really avoiding the issue at that point :) i was confused as you made it seem like there was an easier solution. we also need to re-create _cleanup_header_comment as it's not meant to be used outside of core. want to submit a PR?

[derkjn]

Sorry I was in a bit of a rush and didn't make myself really clear :)
I'll play around with this a bit and see if I can get it to work properly. Once tested I'll post my results here and if we're all happy with it, more than happy to submit a PR :)

[derkjn]

Alright so after playing with it for a bit I got to this working solution:

add_action('admin_init', function () {
    $templateRoot = trailingslashit(get_template_directory());
    $cache_hash = md5(trailingslashit(dirname($templateRoot)) . basename($templateRoot));
    $post_templates = [];
    $path = $templateRoot . 'resources/views/';
    $files = scandir($path);
    foreach ($files as $file) {
        if (is_dir($path . $file)) {
            continue;
        }
        if (!preg_match('|Template Name:(.*)$|mi', file_get_contents($path . $file), $header)) {
            continue;
        }

        $types = array('page');
        if (preg_match('|Template Post Type:(.*)$|mi', file_get_contents($path . $file), $type)) {
            $types = explode(',', _cleanup_header_comment($type[1]));
        }

        foreach ($types as $type) {
            $type = sanitize_key($type);
            if (!isset($post_templates[$type])) {
                $post_templates[$type] = array();
            }

            $post_templates[$type][$file] = trim(preg_replace("/\s*(?:\*\/|\?>).*/", '', $header[1]));
        }
    }
    wp_cache_add('post_templates-' . $cache_hash, $post_templates, 'themes', 1800);
}, 1);

Any feedback is most welcome. If accepted I'm happy to open a PR with it :)

[ciromattia]

The following line
$post_templates[$type][$file] = trim(preg_replace("/\s*(?:\*\/|\?>).*/", '', $header[1]));
should be changed to
$post_templates[$type][$full_path] = trim(preg_replace("/\s*(?:\*\/|\?>).*/", '', $header[1]));
to maintain coherence with wordpress' internal way of setting post template meta - _and_ because changing it to a numerical id breaks ACF way of setting the rule for Page Templates.

[derkjn]

Thanks @ciromattia for pointing that out, code updated 😄

[kimhf]

I get a lot of notices with that code.

Could you change
foreach ($files as $file => $full_path) {
to something like

foreach ($files as $file) {
  if ( is_dir( $path . $file ) ) {
    continue;
  }

and just rename $full_path to $file?

[ciromattia]

@Kimers what kind of notices?

[kimhf]

Warning: file_get_contents(resources/views/.): failed to open stream: Permission denied in app\filters.php

Warning: file_get_contents(resources/views/..): failed to open stream: Permission denied in app\filters.php

Warning: file_get_contents(resources/views/layouts): failed to open stream: Permission denied in app\filters.php

Warning: file_get_contents(resources/views/partials): failed to open stream: Permission denied in app\filters.php

Other then this it seems to work just fine.

[ciromattia]

Strange, are you sure your directories have 755 permissions?

[kimhf]

Have no idea, it is on a WAMP local server, not sure it is even possible to manage permissions that apache would recognize with that or how to set that up. It does not have an issue with the php files, just the folders and "." and ".." (not sure what those are). Seem to me the issue is that we can't just assume that we can use file_get_contents() on everything scandir() returns. The trick for me seems to be to filter out everything else then those php files. is_dir() seems to return true on the "." and ".." and the two folders.

[andrewklau]

Including @kimers changes will introduce a new problem for undefined $full_path variable on child templates

Also the above method breaks child pages that use the js router
ie. a page parent->child used to run JS that was in Parent.js however now the body classes are:

page-template page-template-parent page-template-views page-template-template-languages-blade page-template-parentviewstemplate-languages-blade-php page page-id-72 page-child parent-pageid-67 child

Although perhaps something that could be fixed here instead https://github.com/roots/sage/blob/master/app/filters.php#L8

[andrewklau]

This seems to be a temporary fix for the parent class but probably not the ideal approach:

global $post;
parents = get_post_ancestors($post->ID);
$id = ($parents) ? $parents[count($parents)-1]: $post->ID;
$parent = get_post($id);
$classes[] = $parent->post_name;

[derkjn]

Thanks for the feedback everyone!
@andrewklau I just tested this but I correctly get the class page-parent in my templates as before the folder restructure, so not sure where your problem is originating. The one you posted is the child page classlist which is not supposed to have the page-parent class, but only a reference to the parent id (parent-pageid-67 in your example).

Updated the hook to exclude directories as well 😄

[alexkerber]

Any update with this bug? :/

[vdrnn]

@alexkerber there is a pull request waiting to get merged to master. If you need it now, just get the code from the pull request and implement it yourself real quick, that should solve the problem: https://github.com/roots/sage/pull/1876/files

[rutger1140]

I can confirm it works smoothly with Bedrock and Valet 👌

[retlehs]

fixed by #1877