Direct including of template files

[khacoder]

Initial PR
Issue #74

[grappler]

There should be space between the function name and instead. The error code needs to be updated too.

[grappler]

Would include( get_template_directory() . "/searchform.php" ); and // searchform.php be caught too?

[jrfnl]

Why are you sniffing for T_STRING ?

T_STRING parent, self, etc. identifiers, e.g. keywords like parent and self, function names, class names and more are matched.

See: http://php.net/manual/en/tokens.php

You probably want PHP_CodeSniffer_Tokens::$stringTokens instead of this array.

Or sniffing for include/require tokens, but then again, in that case we would not catch include $pathtosearchform; so even though it's quite heavy, sniffing for the string tokens is probably best.

[jrfnl]

Three things here:

1. trim( $token['content'] . '\"\'' ) => . (dot = concatenation) should be a , (comma = next parameter indicator).
2. Similar to the include/require sniff, it might be better to create a (array) class property to hold the searchform.php to allow for more files to be added in the future. As a value for the array I'd suggest using the alternative get_search_form().
3. In that case, this line will have to be revisited again depending on how the property is set up.

[jrfnl]

The error sentence structure will probably need revisiting if the filename gets turned into a class property.
In that case, the sentence will need placeholders and the $data parameter will need to be passed to the function.

[jrfnl]

Please add some more examples of things which should be ok and which shouldn't. The current examples all use include while that is not what the sniff is sniffing for.

NB: Pseudo code:

$path = get_template_directory() . '/searchform.php' ); // Error.
include $path;

$path = get_childtheme_directory() . "/searchform.php" ); // Error.
$path = get_childtheme_directory() . "/my-completely-different-searchform.php" ); // OK (will currently throw an error though).

$dummy_text_on_example_page = ".... so if you want to add an extra search form, just use `include searchform.php` somewhere in the template code..."; // What to do about things like this ? Should the sniff trigger on it or not ?

[grappler]

@jrfnl I have changed the Sniff to be a bit more general and check that the template functions are used instead of directly including the files. I have added a check for get_head(), get_footer() and get_sidebar(). The check is not sophisticated enough to check for variations of header.phpand sidebar.php but I think this check should be enough.

With the new changes to the sniff the extra tests that you mentioned are being passed.

I wonder if we should merge this sniff with WordPress_Sniffs_Theme_FileIncludeSniff

@justintadlock Could you just check that the sniff is complete?

[justintadlock]

Don't you mean get_header() and get_footer() instead of wp_head() and wp_footer() in this code?

protected $template_files = array(
    'searchform.php' => array(
        'alt' => 'get_search_form()',
    ),
    'header.php' => array(
        'alt' => 'wp_head()',
    ),
    'footer.php' => array(
        'alt' => 'wp_footer()',
    ),
    'sidebar.php' => array(
        'alt' => 'get_sidebar()',
    ),
);

[grappler]

Yes you are right, sorry, I will correct that.

[jrfnl]

I wonder if we should merge this sniff with WordPress_Sniffs_Theme_FileIncludeSniff

I'm in two minds about that. On the one hand, both sniffs look for the inclusion of files.
On the other hand, they have different error levels, different search patterns and different implications.
This sniff is searching for very specific filenames, will also catch them when used in combination with a variable name like $file = 'searchform.php'; include $file; and will suggest an directly implementable alternative.
The include/require sniff is a basic sweep of all such calls and needs manual inspection to determine whether the usage is valid, aka warning.

I'm leaning towards leaving them as separate sniffs as the intend of the sniffs is different.

I have changed the Sniff to be a bit more general and check that the template functions are used instead of directly including the files. I have added a check for get_head(), get_footer() and get_sidebar().

👍

The check is not sophisticated enough to check for variations of header.php and sidebar.php but I think this check should be enough.

That wouldn't be too hard to implement actually. Foreach loop combined with a regex pattern could solve that quite easily.
If you need a hand with the regex, let me know or add some examples of variations you'd be looking for to the test case file.

[grappler]

I have added the additional tests, could use help with the regex.

[grappler]

I have run the sniff on the some of the theme of the repo and got some false positives

1. from https://themes.trac.wordpress.org/browser/catalog-me/1.0.10/product-adder.php

$page_template           = get_page_template_slug( $page_id );
$id                                      = 'container_sidebar_wrap';
if ( $page_template == 'sidebar-page.php' ) {
        $id = 'no_sidebar_wrap';
}

I was surprised that this was caught. After looking at the code it is shows another issue. sidebar-page.php is a page template but could clash if get_sidebar( 'page' ) was used.

2. from https://github.com/justintadlock/hybrid-core/blob/4.0/inc/template.php#L102_L214

	if ( $name ) {
		$templates[] = "header-{$name}.php";
		$templates[] = "header/{$name}.php";
	}
	$templates[] = 'header.php';
	$templates[] = 'header/header.php';
	locate_template( $templates, true, false );

3. from https://themes.trac.wordpress.org/browser/attitude/3.0.8/functions.php

        require_once( ATTITUDE_STRUCTURE_DIR . '/header-extensions.php' );
        require_once( ATTITUDE_STRUCTURE_DIR . '/sidebar-extensions.php' );
        require_once( ATTITUDE_STRUCTURE_DIR . '/footer-extensions.php' );

4. from https://themes.trac.wordpress.org/browser/altitude-lite/1.1/cyberchimps/init.php

        require_once( $hooks_path . 'header-hooks.php' );
        require_once( $hooks_path . 'footer-hooks.php' 

I think we can prevent these if we only check the files in the root of the theme and exclude any folder and exclude functions.php

[jrfnl]

I have run the sniff on the some of the theme of the repo and got some false positives

I don't think limiting the sniff to certain files is the way to go per se.
Let's take a step back and try to get a crystal clear definition of what this sniff should do and catch.
Let's write unit tests to match.
And only once that's done, let's look at the actual sniff again.

[jrfnl]

Closing this PR in favour of continuing the discussion in the original issue #74.

Once that discussion has run its course, we can always reopen.