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 🤞 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? 😄

[jrfnl]

I'm not 100% sure if I should check for the comment at all, or just check for prefixed file names.

I don't think you should as I don't think WP core does when including the file, so checking for the comments would lead to false negatives. (should be verified though, I haven't dug that deep)

If you remove that check, you can change the register() method to:

		return array(
			\T_OPEN_TAG,
			\T_OPEN_TAG_WITH_ECHO,
		);

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

Whatever you prefer, I'm not fussed about it.

Only thing to consider here as more sniffs may need to (or already) use this method: if this method is copied in, should it be in this sniff or would it be better to add it as a static method in a separate WPThemeReview\Utils\Common class ?

[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 🙂 ? #213 (comment)

[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 🙂

[jrfnl]

@dingo-d OK, so I've had another look.

Quite some remarks are just small points, dotting the i's and crossing the t's, but there are still a few more pertinent issues which should be solved too.

[dingo-d]

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

[jrfnl]

@dingo-d We're very, very nearly there. So close....

I have just two questions left from this last review:

1. While the WP convention is to always use lowercase file names, not all themes may comply with this.
   • I haven't checked the WP core code, but I'm wondering if WP does a case sensitive or case insensitive check for the prefix.
     If case-sensitive, all is good.
     If case-insensitive, the sniff may need to account for that.
   • Independently of the answer, it may be a good idea to add the names of the WP core functions which do the "which template file should be loaded" negotiations related to this sniff to the class docblock in @see tags with possibly a @link tag to the WP core file in trac containing these methods or the developer.wp.org docs.
     This would help for anyone looking at the sniff in the future to know where to look to verify if the sniff needs an update or not.
2. The tests look good.
   I'm just wondering if we account for all situations, especially regarding the explode().
   • I think adjusting page-contact.inc to page-contact-form.inc should account for the possibility of multiple dashes in the file name and the sniff handling this correctly (though this is not tested automatically, but would need a visual check of the test results when the sniff is run on the test files.
   • You may want to add a page.inc, category-.inc and a -tag.inc test file to cover the check for no dash or empty parts on line 128-131. None of these should throw an error.

Once the above has been addressed, can you rebase the PR on develop and squash the commits ? Or would you like me to do so ?

[jrfnl]

Should this account for potentially mixed case template file names ?

Suggested change

	if ( isset( $this->reserved_file_name_prefixes[ $file_parts[0] ] ) ) {
	if ( isset( $this->reserved_file_name_prefixes[ strtolower( $file_parts[0] ) ] ) ) {

[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 #213 (comment) 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. 👍

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?

👍

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

👍

[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 👍

[jrfnl]

Yeah! Merged 🎉

[dingo-d]

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