Warning for using include or require #67

khacoder · 2016-08-19T23:32:36Z

Issue #66
Check if include(_once) or require(_once) is used, and issue a warning to see if get_template_part() or locate_template() is more appropriate.

grappler · 2016-09-18T14:13:53Z

WordPress/Sniffs/Theme/FileIncludeSniff.php

+		if ( 'functions.php' !== $fileName ) {
+			foreach ( $checks as $check ) {
+				if ( false !== strpos( $token['content'], $check ) ) {
+					$phpcsFile->addWarning( 'The theme appears to use include or require. If these are being used to include separate sections of a template from independent files, then <strong>get_template_part()</strong> should be used instead.' , $stackPtr, 'FileIncludeCheck' );


'Check that ' . $check . ' is not being used to load template files. <strong>get_template_part()</strong> should be used to load template files.'

grappler · 2016-09-18T14:14:34Z

WordPress/Sniffs/Theme/FileIncludeSniff.php

+
+		$fileName = basename( $phpcsFile->getFileName() );
+
+		$checks = array(


It would be good to rename the variable to make it clearer like $includes

grappler · 2016-09-18T14:14:59Z

WordPress/Sniffs/Theme/FileIncludeSniff.php

+		$fileName = basename( $phpcsFile->getFileName() );
+
+		$checks = array(
+			'include"',


Is there supposed to be a " at the end?

grappler · 2016-09-18T14:15:48Z

WordPress/Sniffs/Theme/FileIncludeSniff.php

+ * @package   PHP_CodeSniffer
+ * @author    khacoder
+ */
+class WordPress_Sniffs_Theme_FileIncludeSniff extends WordPress_AbstractThemeSniff {


Does this class need to extend the WordPress_AbstractThemeSniff?

jrfnl · 2016-10-11T16:48:12Z

WordPress/Sniffs/Theme/FileIncludeSniff.php

+
+		$fileName = basename( $phpcsFile->getFileName() );
+
+		$checks = array(


This array can be removed. Just like the foreach/strpos below.

You are sniffing for the correct tokens, so no need to check their content.

jrfnl · 2016-10-11T16:48:35Z

WordPress/Sniffs/Theme/FileIncludeSniff.php

+		);
+
+		if ( 'functions.php' !== $fileName ) {
+			foreach ( $checks as $check ) {


No need for this foreach nor for the strpos()

jrfnl · 2016-10-11T16:49:15Z

WordPress/Sniffs/Theme/FileIncludeSniff.php

+ * @category PHP
+ * @package  PHP_CodeSniffer
+ * @link     https://make.wordpress.org/core/handbook/best-practices/coding-standards/
+ */


Please update the doc block to comply with the current standards.

jrfnl · 2016-10-11T16:49:19Z

WordPress/Sniffs/Theme/FileIncludeSniff.php

+ * @category  PHP
+ * @package   PHP_CodeSniffer
+ * @author    khacoder
+ */


Please update the doc block to comply with the current standards.

jrfnl · 2016-10-11T16:49:26Z

WordPress/Tests/Theme/FileIncludeUnitTest.php

+ * @category PHP
+ * @package  PHP_CodeSniffer
+ * @link     https://make.wordpress.org/core/handbook/best-practices/coding-standards/
+ */


Please update the doc block to comply with the current standards.

jrfnl · 2016-10-11T16:49:31Z

WordPress/Tests/Theme/FileIncludeUnitTest.php

+ * @category PHP
+ * @package  PHP_CodeSniffer
+ * @author   khacoder
+ */


Please update the doc block to comply with the current standards.

jrfnl · 2016-10-11T16:49:35Z

WordPress/Tests/Theme/FileIncludeUnitTest.php

+	 * should represent the number of errors that should occur on that line.
+	 *
+	 * @return array(int => int)
+	 */


Please update the doc block to comply with the current standards.

jrfnl · 2016-10-11T16:49:39Z

WordPress/Tests/Theme/FileIncludeUnitTest.php

+	 * should represent the number of warnings that should occur on that line.
+	 *
+	 * @return array(int => int)
+	 */


Please update the doc block to comply with the current standards.

jrfnl · 2016-10-11T16:51:45Z

WordPress/Sniffs/Theme/FileIncludeSniff.php

+			'require_once',
+		);
+
+		if ( 'functions.php' !== $fileName ) {


I would make functions.php a protected property in the form of an array ( array( 'functions.php' => true, ) ). That way if at some point other files should be excluded, the sniff is already prepared for that.

You can then change this to a ! isset().

I don't fully understand what you mean with ! isset().

I would have done

$files = array( 'functions.php', ); if ( in_array( $fileName, $files ) ) { }

There are two reasons for this comment:

Other similar arrays in WPCS already follow the pattern I described, so it's better to stay in line with that.

The reason WPCS does this is code-technical. isset() is a language construct which is blazing fast. in_array() is a function call. So to leverage ìsset(), the array key needs to be what you are actually looking for, which is why you'd use an array pattern like this:

$class_property = array( 'functions.php' => true, );

Can you share how the if statement should be?

Of course - as in this case we want to show the error if the file is not in the file whitelist:

if ( ! isset( $this->class_property[ $filename ] ) ) { // Show error. }

For the property name, $file_whitelist might be a good one ;-)

jrfnl · 2016-10-11T18:01:05Z

WordPress/Sniffs/Theme/FileIncludeSniff.php

+	 */
+	public function register() {
+		return array(
+			T_REQUIRE,


One more: you probably should use PHP_CodeSniffer_Tokens::$includeTokens here. You can just straight return it.

grappler · 2016-10-13T13:24:15Z

I have made the requested changes.

jrfnl · 2016-10-13T15:53:35Z

@grappler Nearly there.

$file_blacklist is now still a variable within the function while it does not need to be redefined every time the function is called, it doesn't change between function calls after all, so it should be a class property which is defined only once (if we could use PHP 5.6 we could even make it a class constant, unfortunately arrays can not be used with class constants until then).

Secondly, it is not a blacklist, but a whitelist. If the file name is in the whitelist of files which are logical places/in which a require/include is allowed to exist, no warning will be thrown.

Issue #66 Check for use of include or require and if used issue a warning to check if get_template_part() is more appropriate.

grappler · 2016-10-13T22:45:01Z

I hope it is all good now 😄

jrfnl · 2016-10-13T22:59:12Z

WordPress/Sniffs/Theme/FileIncludeSniff.php

+	 *
+	 * @var array
+	 */
+	public $file_whitelist = array(


The property should have either protected or private as visibility as any properties which are public in a PHPCS sniff can be changed/overruled from the ruleset.

I can imagine a situation where we would allow people to add more files, but in that case, this would still need to be a protected/private property and we would add a $custom_whitelist property which people could pass via the ruleset and which would then be merged.

jrfnl · 2016-10-13T23:05:36Z

WordPress/Sniffs/Theme/FileIncludeSniff.php

+	 *
+	 * @var array
+	 */
+	public $supportedTokenizers = array(


No need to change this, but as a general remark - I believe the default in PHPCS is PHP and adding the property is only needed when you want to sniff JS/CSS or a combination of these, though I'd have to double check the default to be sure.
(If it is the default, then there is no need for the property)

jrfnl · 2016-10-13T23:16:26Z

WordPress/Sniffs/Theme/FileIncludeSniff.php

+		$file_name = basename( $phpcsFile->getFileName() );
+
+		if ( ! isset( $this->file_whitelist[ $file_name ] ) ) {
+			$phpcsFile->addWarning( 'Check that ' . $token['content'] . ' is not being used to load template files. <strong>get_template_part()</strong> should be used to load template files.' , $stackPtr, 'FileIncludeFound' );


Must have missed this earlier. Sorry.
The addWarning()/addError() methods have native sprintf() functionality build in. To use it all you need to do is use the normal %s placeholders in the error message and pass an array of replacements as the fourth parameter to the methods.
Also: as the output is generally command line, using HTML in the error message is confusing.

Like so:

$phpcsFile->addWarning( 'Check that %s is not being used to load template files. "get_template_part()" should be used to load template files.' , $stackPtr, 'FileIncludeFound', array( $token['content'] ) );

jrfnl · 2016-10-13T23:18:26Z

WordPress/Tests/Theme/FileIncludeUnitTest.inc

+include('/file.ext');
+include_once('/file.ext');
+require('/file.ext');
+require_once('/file.ext');


include and require are language constructs and do not need parenthesis, so it might be good to have some examples with more syntax variations:

include dirname( __FILE__ ) . '/myfile.php'; require_once $path_to_file;

jrfnl · 2016-10-13T23:19:28Z

Nearly there, just dotting the i's now...

grappler · 2016-10-14T02:49:01Z

Getting closer minute by minute.

jrfnl · 2016-10-22T16:39:30Z

WordPress/Sniffs/Theme/FileIncludeSniff.php

+	 */
+	public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) {
+		$tokens = $phpcsFile->getTokens();
+		$token  = $tokens[ $stackPtr ];


You don't need those two variables until line 55, so they could go into the if instead of always being created.

grappler reviewed Sep 18, 2016

View reviewed changes

grappler force-pushed the feature/theme-review-sniffs branch 2 times, most recently from 563cd1d to 6671823 Compare September 27, 2016 12:28

jrfnl requested changes Oct 11, 2016

View reviewed changes

Warning for using include or require

05981fa

Issue #66 Check for use of include or require and if used issue a warning to check if get_template_part() is more appropriate.

jrfnl requested changes Oct 13, 2016

View reviewed changes

Update inline doc & improve check

3a3b403

grappler self-assigned this Oct 17, 2016

grappler added the pr: needs review label Oct 17, 2016

jrfnl reviewed Oct 22, 2016

View reviewed changes

jrfnl approved these changes Oct 22, 2016

View reviewed changes

jrfnl merged commit d660bf8 into WPTT:feature/theme-review-sniffs Oct 22, 2016

jrfnl removed the pr: needs review label Oct 22, 2016

grappler mentioned this pull request Dec 3, 2016

Warning for using include and require #66

Closed

4 tasks

grappler mentioned this pull request Dec 18, 2016

Add sniffs for the theme checks WordPress/WordPress-Coding-Standards#578

Closed

72 tasks

jrfnl added this to the 0.1.0 milestone Aug 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warning for using include or require #67

Warning for using include or require #67

khacoder commented Aug 19, 2016

grappler Sep 18, 2016

grappler Sep 18, 2016

grappler Sep 18, 2016

grappler Sep 18, 2016

jrfnl Oct 11, 2016

jrfnl Oct 11, 2016

jrfnl Oct 11, 2016

jrfnl Oct 11, 2016

jrfnl Oct 11, 2016

jrfnl Oct 11, 2016

jrfnl Oct 11, 2016

jrfnl Oct 11, 2016

jrfnl Oct 11, 2016

grappler Oct 12, 2016

jrfnl Oct 12, 2016 •

edited

Loading

grappler Oct 12, 2016

jrfnl Oct 12, 2016 •

edited

Loading

jrfnl Oct 11, 2016

grappler commented Oct 13, 2016

jrfnl commented Oct 13, 2016 •

edited

Loading

grappler commented Oct 13, 2016

jrfnl Oct 13, 2016

jrfnl Oct 13, 2016

jrfnl Oct 13, 2016

jrfnl Oct 13, 2016

jrfnl commented Oct 13, 2016

grappler commented Oct 14, 2016

jrfnl Oct 22, 2016


		$fileName = basename( $phpcsFile->getFileName() );

		$checks = array(

Warning for using include or require #67

Warning for using include or require #67

Conversation

khacoder commented Aug 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grappler commented Oct 13, 2016

jrfnl commented Oct 13, 2016 • edited Loading

grappler commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl commented Oct 13, 2016

grappler commented Oct 14, 2016

Choose a reason for hiding this comment

jrfnl Oct 12, 2016 •

edited

Loading

jrfnl Oct 12, 2016 •

edited

Loading

jrfnl commented Oct 13, 2016 •

edited

Loading