Fix issue on page preview

[slackday]

Fixed an issue where the wrong page hierarchy would be returned when in preview mode of a newly created page.

This was a problem when you would create a new page, selected a custom template and previewed it. The hierarchy would still be Page/Singular due to $post->post_name not being set.

[gmazzap]

First of all, thanks for this @slackday

There're two things that I would like to change before merging. If you don't have time I'll change them, otherwise if you push a new commit, I'll merge your PR.

First, a very minor thing. For the creation of "empty" post, there's no need to pass 'title' as empty string, because that is already set on WP_Post (https://github.com/WordPress/WordPress/blob/master/wp-includes/class-wp-post.php#L71).

After that, you are setting the variable $pagename everytime the query is for preview and pagename is not set. This means that we will have a $pagename variable set when every post is previewed, not only when 'pages' are previewed.
$pagename is a query var specific to post with type "page" and, when not empty, could cause a template to be loaded by post slug, something that happen in WP only for pages, so we need to be sure that $pagename is set only when post type is "page".
When you preview a newly created page, the query variable is page_id which is not used when previewing other post types (that use the var p).

So I think that we could fix the bug, by only adding an if for this special case, e.g.:

$pagename = $query->get('pagename');
if (!$pagename && $query->is_preview() && $query->get('page_id')) {
    $pagename = sanitize_title($post->post_title);
}

Opinions? And thanks again for your work.

[gmazzap]

Also: have you tested how WordPress behave in this case? Reading WordPress code, it seems that WordPress does not have special cases for preview.

[slackday]

First, a very minor thing. For the creation of "empty" post, there's no need to pass 'title' as empty string, because that is already set on WP_Post

Doh, of course the WP_Post object takes care of this. Thanks!

After that, you are setting the variable $pagename everytime the query is for preview and pagename is not set. This means that we will have a $pagename variable set when every post is previewed, not only when 'pages' are previewed.

Also good feedback. I assumed BranchPage class and it's leaves() method only ran on Pages but I didn't look into it. Looks good to me I'll make the changes as a new commit.

Also: have you tested how WordPress behave in this case? Reading WordPress code, it seems that WordPress does not have special cases for preview.

When investigating this issue I started with a clean WP install. Then I added a Controller and I could narrow it down. On a clean WP install templates behave as expected.

But now when I'm looking at the WordPress code for get_page_template() I think I have a lead to what's going on. Because the trail which lead me here all started with the body_class() function.

The Controller mentioned above relies on body classes to load data for each route in WP. Looking at the Core body_class() function it does not rely on is_preview but it also does not rely on pagename.

So what we have is an edge case where the Controller is assuming (through Hierarchy) that the classes are the same as returned by body_class which they wouldn't be because one assumes it's a page and the other is agnostic of post_type and checks is_page_template().

I think this solution will work with the changes you suggested above. What I don't know is why this separation exists in WP Core. What are your thoughts? I'm not so "well traveled" in WP Core code so I might sound like I know more than I do 😄

[codecov-io]

Codecov Report

Merging #16 into master will decrease coverage by 0.19%.
The diff coverage is 50%.

@@            Coverage Diff            @@
##           master      #16     +/-   ##
=========================================
- Coverage   90.31%   90.12%   -0.2%     
=========================================
  Files          29       29             
  Lines         413      415      +2     
=========================================
+ Hits          373      374      +1     
- Misses         40       41      +1

Impacted Files	Coverage Δ
src/Branch/BranchPage.php	96% <50%> (-4%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 239f9bb...fbcc8ea. Read the comment docs.

[gmazzap]

Thanks @slackday your code looks fine to me now. I want to look how core do it, because I want hierarchy to be in sync with core template hierarchy.

Regarding body class, for some reason the page name is never part of the classes, so WP don't bother checking pagename. Also hierarchy needs to resolve to one thing, body class can accumulate. Btw, I'll do some check agains core and let you know.

Thanks again.

[slackday]

Cool! I'd like to know if you find anything. Also I'd tell if I figure something out.

Thanks for great code and glad I could contribute!

[gmazzap]

So it seems that WordPress does not use pagename when in preview of no saved posts, nevertheless the page template is took into consideration.

Basically, there's a bug in Hierarchy (compared to how WP works) that make Hierarchy skip page template in case no pagename is there. This needs to be fixed. Moreover, I saw that in latest version of WP get_page_template() have changed adding another template that is "page-{$pagename_decoded}.php" where $pagename_decoded = urldecode( $pagename );.

So I BranchPage needs to be refactored to be more inline with latest WP. I did the refactoring including a unit test into the branch https://github.com/Brain-WP/Hierarchy/tree/issue-16 that is branched from dev so it includes changes not yet released (most relevant support for page templates for CPT).

@slackday Would you be so nice to test if code in that branch solves your use case?

[slackday]

Tested it. It works as expected for my use case. $pagename === '' now while in preview mode on a new page but the correct template is still returned 👍

Also tested this in the context of the Controller and it also works. Nice & quick work! I like this code more than the solution I posted before.

[gmazzap]

Thanks @slackday for yor time and you input. Sorry I could not merge your code, but there was the need a complete rewrite and it was faster to me do it.

I've merged the branch in dev. I am still waiting someone to help in testing #15 to go forward with merge in master (and new release).
If you have will and time...