Add Reserved Template Name Sniff

[dingo-d]

This is a PR for #212 issue.

I'm sure more test cases would be a good idea, and I'm not 100% sure if I should check for the comment at all, or just check for prefixed file names.

One thing that I had to add was strip_quotes method from the WPCS Sniff abstract class.

Should I have just extended that sniff instead of implementing the Sniff interface from PHPCS (seeing how WPCS's abstract Sniff class already implements it).

[jrfnl]

One thing that I had to add was strip_quotes method from the WPCS Sniff abstract class.

Should I have just extended that sniff instead of implementing the Sniff interface from PHPCS (seeing how WPCS's abstract Sniff class already implements it).

If things go as planned :crossed_fingers: and PR squizlabs/PHP_CodeSniffer#2456 will be accepted, PHPCS 3.5.0 will contain that function natively: https://gist.github.com/jrfnl/16e1d64d9ec8efec7729e9bfbb05201a#in-php_codesnifferutilsniffstextstring

[dingo-d]

Oh the mega PR 😄

So should I leave it as is and refactor after that, or extend the WPCS Sniff and refactor after that? 😄

[dingo-d]

I've fixed the issues, and changed the code as suggested, but I won't commit until I get the approval on the comments I've left (so as to not pollute the commit history) 🙂

I'm not sure we need the util class in the standards, as most of the time we're extending WPCS sniffs (which are extending the WPCS Sniff class). Plus if everything goes as planned, the helper should end up in the PHPCS, and then it would be easy to just remove the helper method from the sniff and use it from the upstream 🙂

EDIT:

@jrfnl Did you had the chance to go through my comment 🙂 ? https://github.com/WPTRT/WPThemeReview/pull/213#discussion_r283122168

[jrfnl]

Hope I've caught all the comments you wanted a response to. Let me know if I missed any.

[dingo-d]

First, the regex comment blew my mind, from 13 passes to 4, and then the fact that I didn't need regex in the first place. Never would have thought of that 😄 I'm super grateful for your help, remind me to buy you a beer at WCEU (the least I can do) 😄

[dingo-d]

@jrfnl This is also ready for a final review 🙂

[dingo-d]

I've added your suggestion, is all ok now? 🙂

[dingo-d]

@jrfnl From what I gathered, the template-loader.php which is responsible for loading the templates in WordPress, has a series of if/elseif checks that will determine if certain queried object is a category, 404 page, search page etc.

For instance the get_category_template() function looks like this

function get_category_template() {
    $category = get_queried_object();

    $templates = array();

    if ( ! empty( $category->slug ) ) {

        $slug_decoded = urldecode( $category->slug );
        if ( $slug_decoded !== $category->slug ) {
            $templates[] = "category-{$slug_decoded}.php";
        }

        $templates[] = "category-{$category->slug}.php";
        $templates[] = "category-{$category->term_id}.php";
    }
    $templates[] = 'category.php';

    return get_query_template( 'category', $templates );
}

So the answer to the first question is that core expects lowercase filenames for the core templates. Maybe I could link the template hierarchy docs for this (in the @see docblock), or do you want me to link the 4 reserved file name prefixes ( get_page_template(), get_category_template(), get_tag_template(), get_author_template()) documentations?

I have added the suggested test files and all is fine 🙂

EDIT:

I've added the links to the four functions that we are checking for, and the template hierarchy docs. This should be fine?

[jrfnl]

So the answer to the first question is that core expects lowercase filenames for the core templates.

Thank you for checking that. In that case, the change I suggested in https://github.com/WPTRT/WPThemeReview/pull/213#discussion_r301039976 wasn't necessary and should be undone and we should possibly have a test case file with mixed case in the file name which shouldn't trigger the sniff, like Page-something.inc.

Sorry for the confusion, but I'm very glad you checked as this thread now documents your findings about the check within WP for these files names being case-sensitive. :+1:

So, aside from this, all looks absolutely perfectly fine and ready for merge.

Maybe I could link the template hierarchy docs for this (in the @see docblock), or do you want me to link the 4 reserved file name prefixes ( get_page_template(), get_category_template(), get_tag_template(), get_author_template()) documentations?

I've added the links to the four functions that we are checking for, and the template hierarchy docs. This should be fine?

:+1:

I have added the suggested test files and all is fine slightly_smiling_face

:+1:

[joyously]

Does it matter that only some operating systems are case sensitive, when looking for the file?

[dingo-d]

I don't think this should be an issue, as the core is handling the file loading, so if you have case sensitive filename it won't load it as a template (Page-info.php for instance).

[joyously]

I'm just saying that on a Windows server, looking for 'page-' would match 'Page-', but not on a Linux server.

[jrfnl]

I don't think this should be an issue, as the core is handling the file loading, so if you have case sensitive filename it won't load it as a template (Page-info.php for instance).

Are we sure about that ? Has someone tried/tested this with a real theme on a non-Linux system ?

[jrfnl]

@dingo-d I'm happy to merge this as it is at the moment. Well done! Great work.

If someone has a chance to check out the whole case-sensitivity (or not) of template loading in core at some point on multiple OSes in more detail and the check would turn out to need to be case-insensitive, an issue can be opened to update this PR.

Does that sound reasonable to you ?

[dingo-d]

On Windows machine even if you name a template Page-info.php, WordPress will load it like if you were to name it page-info.php, while on my mac the Page-info.php will get ignored, and page-info.php will not get ignored.

So we need to take this into the account while sniffing, right?

[jrfnl]

On Windows machine even if you name a template Page-info.php, WordPress will load it like if you were to name it page-info.php, while on my mac the Page-info.php will get ignored, and page-info.php will not get ignored.

So we need to take this into the account while sniffing, right?

If that's the case, then yes, we should.

Implementing my earlier suggestion related to this question should take care of that + the Page-info.inc test case file will need to be added to the errors list in the unit test file in that case.

[jrfnl]

I think this whole case-sensitive template file names discussion was very useful to have and it does raise another question: whether there should be rule that template files should always be in lowercase to prevent issues with templates loading on Windows, but not loading on Linux/Mac (and a sniff to verify this).

Not something to be addressed in this sniff, but someone may want to open a new issue about this and this should possibly be discussed in a TRT meeting ?

[dingo-d]

Squashed all the commits and added the changes you mentioned.

I've brought your comment up with the leads, and I'll mention it on todays meeting if there will be the time because this could be added to the themes handbook which we'll have access to :+1:

[jrfnl]

Yeah! Merged :tada:

[dingo-d]

Awesome 😄 Thanks! We're nearing the 0.2.0 release 😄