Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix template loading #145

Closed
wants to merge 3 commits into from
Closed

Fix template loading #145

wants to merge 3 commits into from

Conversation

jmslbam
Copy link
Contributor

@jmslbam jmslbam commented May 2, 2019

Issue at hand

front-page.twig wasn't used when a page was set to "front-page" via the WordPress dashboard. This means the Theme Template Hierarchy wasn't followed. We also noticed this with the bug that Niels encountered when a Archive was empty and Clarkson previously fell back to 'index'.

Fixes

  1. Don't replace the minus from the basename of the template. Because this way 'front-page.twig' can be used so the correct template hierarchy can be followed.

  2. Fix finding the correct twig template file.
    Don't incorpate own custom logic because the third $templates parameter contains correct template name to search for according to the Theme Hierarchy.
    This method, using the $templates array, also considers the egde-case / bug that @NielsdeBlaauw encoutered: Load the correct archive.twig template when you visit the archive page but there are not Posts in that archive. This because WordPress already take this in account and we now really follow the WordPress flow by using $templates array.

Based on #99 but with some extra documentation, checks and fallbacks. Also tested with variety of possible templates.

Backwards compatibilty

Is this code backwards compatible. I do think so, because the issue of not being able to use front-page.twig was often fixed by using a custom page template and applying that to the page. But please enlighten me if that's not the case.

jmslbam added 3 commits May 2, 2019 21:38
Because this way 'front-page.twig' can be used so the correct template hierarchy can be followed.
Don't incorpate own custom logic because the third $templates parameter contains correct template name to search for according to the Theme Hierarchy.
This method, using the $templates array, also considers the egde-case / bug that @NielsdeBlaauw encoutered: Load the correct archive.twig template when you visit the archive page but there are not Posts in that archive. This because WordPress alraedy take this in account and we now really follow the WordPress flow by using $templates array.

Based on #99 of @christiaanstijnen and @Willemijnr. Thank you!
@NielsdeBlaauw NielsdeBlaauw self-assigned this May 3, 2019
Copy link
Member

@menno-ll menno-ll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment in code line 270: remove the special exception

if ( 'page' === $type && ! isset( $templates[ "{$type}" ] ) && isset( $templates['singular'] ) ) {
return $templates['singular'];
if ( 'page' === $type && ! isset( $twig_templates['page'] ) && isset( $twig_templates['singular'] ) ) {
$type = 'singular';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this major exception.
The purpose of this commit is to let clarkson-core work more closely to the WP templating structure, while this function is moving away from that.
Also, this only works for pages this way, while posts also have the singular template in the WP hierarchy.
Unfortunately building this for posts as well will also result in inconsistent results because of custom post types not having the same behaviours then.

This exception feature is not documented anywhere, so the chances are slim that it will break anything on existing sites.

@NielsdeBlaauw
Copy link
Member

Closed by #183

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants