Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/templates/required #214

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Feature/templates/required #214

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
355f42f
add body_class/language_attributes sniffs
timelsass May 17, 2019
f6dc4f2
add test cases
timelsass May 17, 2019
f521b85
add required function unit test
timelsass May 17, 2019
6c4c1bc
add sniff for lang attributes and body class
timelsass May 17, 2019
3226883
pass build
timelsass May 17, 2019
c19ea43
code review updates
timelsass May 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
231 changes: 231 additions & 0 deletions WPThemeReview/Sniffs/Templates/RequiredFunctionSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,231 @@
<?php
/**
* WPThemeReview Coding Standard.
*
* @package WPTRT\WPThemeReview
* @link https://github.com/WPTRT/WPThemeReview
* @license https://opensource.org/licenses/MIT MIT
*/

namespace WPThemeReview\Sniffs\Templates;

use PHP_CodeSniffer\Sniffs\Sniff;
use PHP_CodeSniffer\Files\File;
use PHP_CodeSniffer\Util\Tokens;

/**
* Ensures that body_class() is called if theme adds a <body> tag.
*
* @link https://make.wordpress.org/themes/handbook/review/required/#templates
*
* @since 0.1.0
timelsass marked this conversation as resolved.
Show resolved Hide resolved
*/
class RequiredFunctionSniff implements Sniff {

/**
* Sniff Settings
*
* @var array
*/
public $tagsConfig = array(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public sniff properties can be changed from a custom ruleset. Was this done intentionally? If not you can change visibility to protected 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I don't know the stance on that for this repo, but it was intentional mostly for my own use case, which probably isn't very common. I figured if I had a need to tweak something, then someone somewhere might as well. I did name this as a "config" to indicate it was able to be configured and is an open point, so others can implement as needed. I generally like to leave configurations open as much as possible because you never know what someone might want to modify 😄, but I completely understand if that's not a standard approach here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timelsass Sorry, but this won't work. Please make this property protected or private.

While you can have public array properties which can be changed from within a ruleset, those can only be simple arrays, not complex multi-level arrays like the one you are using here. And even then, if this were to be public you'd need a validation routine to ensure the user-input received would be usable.

If you'd want to be able to extend the array I suggest making it a protected property so other sniffs can extend this sniff and overload it.

Other than that, I think modular error codes is the way to go and you're already using those, so 👍 .

'body' => array(
'function' => 'body_class',
'attribute' => 'class',
),
'html' => array(
'function' => 'language_attributes',
'attribute' => 'lang',
),
);

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole property can be removed. The PHPCS default is PHP only, so you only need to set this property when you also want to examine JS and/or CSS files.

* Supported Tokenizers
*
* Currently this sniff is only useful in PHP as the required
* functions to call are done in PHP. In testing various
* themes - some had inline comments including `<html>`, and
* were tokenized as T_INLINE_HTML throwing some false positives.
*
* @var array
*/
public $supportedTokenizers = array( 'PHP' );

/**
* Tag being searched.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This description needs to be expanded as from the description alone, it is unclear what the property is for.

Also: should this property maybe be private ?

*
* @var array
*/
protected $tag;

/**
* Returns an array of tokens this test wants to listen for.
*
* @return array
*/
public function register() {
return Tokens::$textStringTokens;
}

/**
* Processes this test, when one of its tokens is encountered.
*
* @param \PHP_CodeSniffer\Files\File $phpcsFile The PHP_CodeSniffer file where the
* token was found.
* @param int $stackPtr The position of the current token
* in the stack.
*
* @return void
*/
public function process( File $phpcsFile, $stackPtr ) {

$tokens = $phpcsFile->getTokens();
$content = $this->clean_str( $tokens[ $stackPtr ]['content'] );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment about the clean_str() method.

$filename = $phpcsFile->getFileName();

// Set to false if it is the first time this sniff is run on a file.
if ( ! isset( $this->tag[ $filename ] ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 on keeping track of which file you are in for matching the tags 👍

I think this can be simplified a little though for better performance.
The way this is currently set up, the $this->tag array can become quite large for themes with a lot of files.

What about just having a string $current_file property and string $last_tag property ?
If the $filename doesn't match $current_file, you assign the new $filename to $this->current_file and can then just clear the $last_tag value or set it to an empty string.
This should lower memory usage when scanning a large theme.

The only thing which would need to be verified in that case would be that this will work OK when running PHPCS with the parallel option. I haven't looked into the implementation of that deeply enough to be sure of whether or not the logic I'm proposing would cause problems when running in parallel.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also wondering if $this->tag[ $filename ] shouldn't be set to false each time the sniff is called anyway as when the closer of the HTML tag is found it should be at false to start with anyway, but if that isn't found (HTML error), you may still have a tag set from a previously call to the sniff.

$this->tag[ $filename ] = false;
}

// Skip on empty.
if ( '' === $content ) {
jrfnl marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When not using the clean_str() method:

Suggested change
if ( '' === $content ) {
if ( '' === trim( $content ) ) {

return;
}

// Set tag class property.
foreach ( $this->tagsConfig as $tag => $settings ) {

// HTML case should be insensitive.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// HTML case should be insensitive.
// HTML case is insensitive.

if ( false !== stripos( $content, '<' . $tag ) ) {
$this->tag[ $filename ] = $this->tagsConfig[ $tag ];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just keep track of the matched $tag ? The $tagsConfig array be accessed from anywhere in this sniff, so there is no need to (partially) duplicate it for each matched tag.

$this->tag[ $filename ]['tag'] = $tag;
break;
}
}

// Skip if not a tag.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Skip if not a tag.
// Skip if not in a tag.

if ( false === $this->tag[ $filename ] ) {
return;
}

// Set vars used for reference.
$tagName = $this->tag[ $filename ]['tag'];
$tagFn = $this->tag[ $filename ]['function'];
$tagAttr = $this->tag[ $filename ]['attribute'];
$pascal = str_replace( ' ', '', ucwords( str_replace( '_', ' ', $tagFn ) ) );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This transformation is not needed unless an error will be thrown. I suggest moving it lower down.

Also have a look at the WPCS string_to_errorcode() method.

$nextPtr = $stackPtr;
$foundFunction = false;
$foundAttribute = false;
$foundEnd = false;

do {
$nextPtrContent = $this->clean_str( $tokens[ $nextPtr ]['content'] );
$nextPtrCode = $tokens[ $nextPtr ]['code'];

// Check for attribute not allowed.
if (
false === $foundAttribute &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small code-consistency note (which isn't checked for by the CS check yet): generally it is considered best practice to have the boolean operators at the start of the line containing the next condition as this improves readability.

While not a formal requirement, as this is consistently done throughout the rest of the code-base, I'd like to suggest complying with that here and in other places in the sniff, as well.

			if (false === $foundAttribute
				&& isset( Tokens::$textStringTokens[ $nextPtrCode ] )
				&& false !== stripos( $nextPtrContent, $tagAttr . '=' )
			) {

in_array( $nextPtrCode, Tokens::$textStringTokens, true ) &&
timelsass marked this conversation as resolved.
Show resolved Hide resolved
false !== stripos( $nextPtrContent, $tagAttr . '=' )
) {
$foundAttribute = true;
}

// Check for required function call.
if (
false === $foundFunction &&
in_array( $nextPtrCode, Tokens::$functionNameTokens, true ) &&
timelsass marked this conversation as resolved.
Show resolved Hide resolved
false !== strpos( $nextPtrContent, $tagFn )
) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code would also match MyClass::language_attributes(), $obj->language_attributes() and My\Name\Space\language_attributes(), none of which are the WP native function which is required to be used.

// Check next non-whitespace token for opening parens.
$next = $phpcsFile->findNext( Tokens::$emptyTokens, ( $nextPtr + 1 ), null, true );

if ( ! $next || ! isset( $tokens[ $next ] ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is sufficient.

Suggested change
if ( ! $next || ! isset( $tokens[ $next ] ) ) {
if ( false === $next ) {

break; // Nothing left.
}

// Verify function( $param = 'optional' ) type.
if ( 'PHPCS_T_OPEN_PARENTHESIS' === $tokens[ $next ]['code'] ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ( 'PHPCS_T_OPEN_PARENTHESIS' === $tokens[ $next ]['code'] ) {
if ( 'T_OPEN_PARENTHESIS' === $tokens[ $next ]['code'] ) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: I'm not sure what you're trying to do here.


// Skip over contents to closing parens in stack.
if ( isset( $tokens[ $next ]['parenthesis_closer'] ) ) {
$nextPtr = $tokens[ $next ]['parenthesis_closer'];
$foundFunction = true;
}
}

continue;
}

// Check for searched tag matched closing bracket.
if (
in_array( $nextPtrCode, Tokens::$textStringTokens, true ) &&
timelsass marked this conversation as resolved.
Show resolved Hide resolved
'>' === substr( $nextPtrContent, -1 )
) {
$this->tag[ $filename ] = false;
$foundEnd = true;
break;
}

// Increment stack to next non-whitespace token.
$next = $phpcsFile->findNext( Tokens::$emptyTokens, ( $nextPtr + 1 ), null, true );

if ( ! $next || ! isset( $tokens[ $next ] ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above. if ( false === $next ) {

break; // Short circuit loop as there's not anything left.
}

$nextPtr = $next;

} while ( false === $foundEnd ); // Loop until matched closing bracket is found for searched tag.

// Required function not found.
if ( false === $foundFunction ) {
$phpcsFile->addError(
"Themes must call {$tagFn}() inside <{$tagName}> tags.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PHPCS addError() and addWarning() methods have build in (s)printf() like functionality. Please use it.
See: https://pear.php.net/package/PHP_CodeSniffer/docs/3.4.2/apidoc/PHP_CodeSniffer/File.html#methodaddError

			$phpcsFile->addError(
				"Themes must call %s() inside <%s> tags.",
				$stackPtr,
				"RequiredFunction{$pascal}",
				array(
					$tagFn,
					$tagName,
				)
			);

$stackPtr,
"RequiredFunction{$pascal}"
);

return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a return here effectively hides one error behind another which will make fixing things unwieldy, so it is better not to do this.

}

// Atrribute is not allowed.
if ( true === $foundAttribute ) {
$phpcsFile->addError(
"Attribute '{$tagAttr}' is not allowed on <{$tagName}> tags. Themes must call {$tagFn}() instead.",
$stackPtr,
"DisallowedAttribute{$pascal}"
);

return;
}
}
Copy link
Contributor

@jrfnl jrfnl Jun 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use return ( $TagEndToken + 1 ); at the very end of the function (outside of the conditions).
This will make the sniff more efficient as PHPCS won't call the sniff again until it's reached that token, preventing it from needlessly (re-)examining strings which have already been examined.


/**
* Cleans string for parsing.
*
* This cleans whitespace chars and single/double quotes
* from string. Primary used to check T_CONSTANT_ENCAPSED_STRING
* and T_DOUBLE_QUOTED_STRING for closing HTML brackets. This is
* because < and > are valid attribute values, and a strpos wouldn't
* be enough.
*
* Strips:
* ' ' : Whitespace
* '"' : double quote
* ''' : single quote
* '\t' : tab
* '\n' : newline
* '\r' : carriage return
* '\0' : NUL-byte
* '\x0B': vertical tab
*
* @param string $str String to clean.
*
* @return string Cleaned string.
*/
private function clean_str( $str ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this could be used in other sniffs, we could do what @jrfnl suggested in the #213 PR about strip_quotes method - create a WPThemeReview\Utils\Common class with these kind of utilities 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funny you mentioned it - I initially made a Util class for this while testing it out, but after looking over WPThemeReview I didn't see one anywhere else, or a common area, so I just figured the "norm" was to keep functions self contained per class. I noticed some WPCS stuff that would be useful, but I haven't worked my way to that level lol.

It looks like we are all trying to address the same issues revolving around T_CONSTANT_ENCAPSED_STRING and T_DOUBLE_QUOTED_STRING though. I don't recall all of the specifics, but I think I needed to trim single quotes in addition to the double quotes added for ensuring the functions were being caught inside of the actual HTML tags where attributes are added, not inside of any quoted areas if it's broken up between HTML/PHP, and accounting for an attribute containing a > character. Not very common but does happen sometimes. Something like this scenario - if I remember correctly (I could be wrong):

echo '<body id="' . $something . '"' . body_class() . "data-title='Some > Title'" . '>';

There were some other weird scenarios with doing things like mixing heredoc/functions(print,
echo, printf etc)/HTML together which doesn't work out perfectly all of the time, but I also think if someone is doing something that far out of the ordinary they obviously have no coding standards or desire to follow any. I thought of a solution afterwards, but it would require doing some weird tracking of the opening/closing quotes, which seemed like a waste of time to cover those fringe cases ( realistically probably 0.000000000001%/never encountering it ).

I'm not sure if this function will be useful anywhere else though, but I could see just the strip_quotes being used commonly. I think a Util class would be nice to have though, and there might be more things as we expand the sniffs further where we realize overlap in some functionality. I honestly have no clue since this is the first time writing a phpcs sniff 😆

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

echo '<body id="' . $something . '"' . body_class() . "data-title='Some > Title'" . '>';

That would break the HTML. The > in Some > Title needs to be encoded as &gt; for this to be valid HTML.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: Util class or something along those lines.

I think this would be a good idea.

If I remember correctly, the reasons why in WPCS all utility methods are in the abstract Sniff class and all classes extend it, is historic.
PHPCS 2.x file auto-loading didn't support helper files as easily, but the PHPCS 3.x file auto-loading is more flexible.

As far as I can see, as long as the file is within the standards directory, is namespaced based on the path and the file name doesn't end on Sniff, this should work in PHPCS 3.x out of the box.

A use statement for the class in sniffs using it should then be able to take care of the rest.

Maybe I suggest a WPThemeReview\Helpers\StringUtils.php file containing a class named StringUtils which would then contain the (static) functions ?
This will allow for adding more Util type classes later for functions addressing other language structures, keeping it all modular instead of having one massive Util class.

Alternatively, for the strip_quotes() method from WPCS, I'd happily accept a PR to make that a static method in WPCS.
For the time being, that should allow you to use it even when a sniff doesn't extend the WPCS Sniff class (to bridge the time until the method is available in PHPCS).

Copy link
Contributor

@jrfnl jrfnl Jun 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but I'm going to be horrible and say this function is a bad idea.

If a text string spans multiple lines, start/end string quotes may be on different lines. By "cleaning" those quotes of the strings in individual lines without matching them, you will be removing legitimate characters which are part of the string itself.

If you want to clear the string quotes of PHP strings:

  1. Gather all the parts of the string first and concatenate them together.
  2. Then run the resulting string through the WPCS strip_quotes function.
  3. DON'T do this for text strings where no quotes are expected, such as T_INLINE_HTML, T_HEREDOC or T_NOWDOC.

return trim( $str, " \"\'\t\n\r\0\x0B" );
}
}
53 changes: 53 additions & 0 deletions WPThemeReview/Tests/Templates/RequiredFunctionUnitTest.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?php
/**
* Unit test for required functions in HTML tags.
*
* This tests language_attributes(), and body_class() requirements.
*/
echo '<html>'; // Bad.
echo '<body>'; // Bad.
?>
<html> <!-- Bad. -->
<html <?php language_attributes(); ?> lang="en-US"> <!-- Bad. -->
<?php
echo '<html lang="en-US" '; // Bad.
language_attributes();
echo '>';
?>
<?php
echo '<html '; // Ok.
language_attributes();
echo '>';
?>
<html <?php language_attributes(); ?> lang="en-US"> <!-- Bad. -->
<?php echo '<body class="body-test" ', body_class(), '>'; // Bad. ?>
<body> <!-- Bad. -->
<?php echo '<body>'; // Bad. ?>
<?php echo '<body class="body-test" ' . body_class() . '>'; // Bad. ?>
<?php echo '<body class="body-test" ', body_class(), '>'; // Bad. ?>
<html <?php language_attributes(); ?>> <!-- Ok. -->
<html <?php language_attributes(); ?> class="html-class"> <!-- Ok. -->
<?php echo '<body id="body-test" ' . body_class() . '>'; // Ok. ?>
<?php echo '<body id="body-test" ', body_class(), '>'; // Ok. ?>
<body <?php body_class(); ?>> <!-- Ok. -->

<?php
// Test common invalid heredoc style.
$html = <<<EOT
<!-- Bad. -->
<body>
...
</body>
EOT;

// Test weird but valid heredoc style.
$html = <<<EOT
<!-- Ok. -->
<body
EOT;
body_class();
$html = <<<EOT
>
...
EOT;
?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Please remove the PHP closing tag from the end of the test file.
  • Please add some tests with multi-line PHP single quoted and double quoted strings.
  • Please add some tests with, for instance, the class attribute with a different HTML tag like <div>.
  • Please add a test with an HTML parse error, like an unclosed tag.
  • Please add some tests with less common HTML formatting like class = "...". If I remember correct HTML ignores most whitespace, so this should still work in HTML.
  • If you are going to keep the file tracking, it may be a good idea to add a (very simple) second test file to verify that the file-based tag remembering is working correctly.

52 changes: 52 additions & 0 deletions WPThemeReview/Tests/Templates/RequiredFunctionUnitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php
/**
* Unit test class for WPThemeReview Coding Standard.
*
* @package WPTRT\WPThemeReview
* @link https://github.com/WPTRT/WPThemeReview
* @license https://opensource.org/licenses/MIT MIT
*/

namespace WPThemeReview\Tests\Templates;

use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;

/**
* Unit test class for the RequiredFunction sniff.
*
* @since 0.1.0
*/
class RequiredFunctionUnitTest extends AbstractSniffUnitTest {

/**
* Returns the lines where errors should occur.
*
* @return array <int line number> => <int number of errors>
*/
public function getErrorList() {
return array(
7 => 1,
8 => 1,
10 => 1,
11 => 1,
13 => 1,
22 => 1,
23 => 1,
24 => 1,
25 => 1,
26 => 1,
27 => 1,
38 => 1,
);
}

/**
* Returns the lines where warnings should occur.
*
* @return array <int line number> => <int number of warnings>
*/
public function getWarningList() {
return array();
}

}