Feature/templates/required #214
Conversation
|
Hmmm, I haven't thought of |
| * | ||
| * @var array | ||
| */ | ||
| public $tagsConfig = array( |
There was a problem hiding this comment.
public sniff properties can be changed from a custom ruleset. Was this done intentionally? If not you can change visibility to protected 🙂
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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 👍 .
| * | ||
| * @return string Cleaned string. | ||
| */ | ||
| private function clean_str( $str ) { |
There was a problem hiding this comment.
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 😆
There was a problem hiding this comment.
echo '<body id="' . $something . '"' . body_class() . "data-title='Some > Title'" . '>';
That would break the HTML. The > in Some > Title needs to be encoded as > for this to be valid HTML.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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:
- Gather all the parts of the string first and concatenate them together.
- Then run the resulting string through the WPCS
strip_quotesfunction. - DON'T do this for text strings where no quotes are expected, such as
T_INLINE_HTML,T_HEREDOCorT_NOWDOC.
|
@timelsass Welcome! I'll try and review your sniff over the next few days, but for now, I just want to leave some comments for you to think about.
I made a very limited start on a sniff for this way back (and got distracted and never got back to it), so I'm very happy to see this being addressed now 😄 All the same, I did set up some unit tests at the time. I haven't checked yet whether your unit tests already cover these cases, nor whether the sniff would handle them correctly, but they may be helpful to you to fine-tune the sniff: <!-- Correct header example. -->
<html <?php language_attributes(); ?>> <!-- OK. -->
<head>
<meta charset="<?php bloginfo( 'charset' ); ?>"><!-- OK. -->
<meta name="description" content="something" />
<meta http-equiv="Content-Type" content="text/html; charset=<?php bloginfo( 'charset' ); ?>" /><!-- OK. -->
<?php wp_head(); ?>
</head>
<body <?php body_class(); ?>><!-- OK. -->
<!-- Correct header example: multi-line calls and such. -->
<html
<?php language_attributes(); ?>> <!-- OK. -->
<head>
<meta
charset="<?php
bloginfo( 'charset' );
?>"><!-- OK. -->
<meta name="description" content="something" />
<meta
http-equiv="Content-Type"
content="text/html; charset=<?php bloginfo( 'charset' ); ?>"
/><!-- OK. -->
<?php wp_head(); ?>
</head>
<body
<?php
body_class();
?>
><!-- OK. -->
<!-- Correct header example: bit convoluted code, but the output would be correct. -->
<html <?= echo get_language_attributes( 'html' ); ?>><!-- OK. -->
<head>
<meta charset="<?= get_bloginfo( 'charset' ); ?>"><!-- OK. -->
<meta name="viewport" content="width=device-width, initial-scale=1">
<link rel="profile" href="http://gmpg.org/xfn/11">
<?php wp_head(); ?>
</head>
<body class="<?php echo implode( ' ', apply_filters( 'theme-body-class', get_body_class( 'additional-class' ), $pageID ) ); ?>"><!-- OK. -->
<!-- Incorrect header: missing function calls. -->
<html> <!-- Bad. -->
<head>
<?php wp_head(); ?>
</head>
<body><!-- Bad. -->
<!-- Incorrect header: attributes filled in. -->
<html lang="en-US"> <!-- Bad. -->
<head>
<meta charset="UTF-8"><!-- Bad. -->
<?php wp_head(); ?>
</head>
<body class="home paged"><!-- Bad. --> |
|
thanks for looking over this @dingo-d and @jrfnl, yeah I started working on meta with charset, but it was a slightly different since it's setting only the property and not the attribute + value(s) on the tag, so I was going to save that for a little later. While doing that though - I saw some common stuff that might be able to be abstracted out, but haven't given much thought as to the best way of doing that still. Yes, I thought about the naming and wasn't sure what to do for it 😄 I just kinda went with what felt okay at the time, but I'm open to any suggestions. I know @dingo-d had talked with me about some naming for the readme file sniffs in WPTRT/theme-sniffer - and also mentioned moving his sniff into I was going based off of trying to keep some kind of convention close to: https://make.wordpress.org/themes/handbook/review/required/#templates - but couldn't figure out a good one. These were some of the ones that I was thinking about, but didn't feel like they really fit quite right for one reason or another:
The first couple sounded more like the tag in the header in context of WordPress theme templates, so I kinda ruled those out - but "TemplateTags" rolls off the tongue nicely. I ended up just using RequiredFunctions since that's what I used when I started, they were functions that are required, and seemed pretty generic that no one would say anything 🤣. I'm not happy with the naming there though, and was hoping someone might have better ideas. Especially as I'll run some tests against what you provided, and update accordingly. The ones in particular are the My thought process was basically along the lines of "use the right functions where they should be used." In that case I believe those situations are still caught correctly. For example, I don't think doing Likewise - I understand why a theme author might do something like what's done in the body classes example, but I don't think that it is code style that should be considered appropriate to pass the sniff. Adding a filter with the theme's prefix, then calling ^^ Those are just my own thoughts, but if everyone else thinks these should be acceptable, I can work on adding those in. There's really not any technical reason why those aren't acceptable ways of handling it from what I can tell since they still get filtered in the On the flip side though, the benefit of adding them in would be that the sniff is expanded out a bit more to cover the functions that only set the values and not just for the attribute + value in the tag. This pretty much brings me back around to why I didn't include the meta charset in this particular sniff in the first place lol. When adding the meta charset I would've completely forgotten about setting it through the content attr with |
|
I'm fine leaving this in As for the name, I was also worried a bit about the naming, since it's open to additions, changes and ambiguity.
Could we then check for the rest of the required functions mentioned in the handbook? Or would this be too much? |
There was a problem hiding this comment.
@timelsass Sorry that it took a while before I could have a proper look.
I've gone through all the discussions above and left some comments here and there. Additionally, here are some more responses based on the above discussions:
I'm not happy with the naming there though, and was hoping someone might have better ideas.
I just had a quick look at what I had in the draft set-up I created way back. Just throwing it into the mix for consideration: UseHTMLAttributeFunctionsSniff or possibly RequiredHTMLAttributeFunctionsSniff
I don't think doing echo get_language_attributes() is necessarily appropriate when a core method exists for echoing the language attributes using the language_attributes() function.
I think the sniffs should encourage the usage of the appropriate methods over considering variations that aren't optimal as being acceptable.
Whether it is appropriate or not is irrelevant for this sniff. Another sniff could be written to look at "code inefficiencies", but those are issues which are completely independent from each other.
If inefficient code is being used, but the correct HTML attributes are being inserted via an appropriate WP native function, this sniff should not throw false positives.
Re: preventing more false positives.
Another example we may need to account for - not necessarily now, but probably should be looked at later -:
/* Template docblock */
$lang_attr = get_language_attributes( 'html' );
$body_classes = get_body_class( 'additional-class' );
$body_classes = apply_filters( 'theme-body-class', get_body_class( 'additional-class' ), $pageID );
<html <?= $lang_attr; ?>><!-- OK. -->
<head>
<?php wp_head(); ?>
</head>
<body class="<?= $body_classes; ?>"><!-- OK. -->Obviously, this can only be checked for if the variables are being defined in the template file, but in that case, a false positive can be prevented.
If this will not be addressed now (and it doesn't need to be), please open an issue for this as a reminder that someone should take a look at this later.
Could we then check for the rest of the required functions mentioned in the handbook?
@dingo-d I think we'd need to decide in that case what this sniff is intended to do.
So far it detects using the appropriate WP native functions to add HTML attribute values.
Functions like wp_head() and wp_footer() are different as they shouldn't be called within an HTML tag, but on their own.
For those, a different kind of logic is needed where you keep track of whether you've, for instance, seen a <body> tag and seen a call to the wp_footer() function before the closing </body> tag.
This gets a lot more difficult if/when templates are split into partials, like header.php, footer.php etc.
So, in my opinion, I think that should be a separate sniff and we should have a careful think about the criteria for when to expect the function calls/throw errors for that sniff.
IMO mixing that logic into this sniff will make this sniff overly complicated and will make debugging things later on, a lot more complicated as well, so I strongly advise separate sniffs.
For whomever will be working on the other sniff - for some example code of how to keep track of whether you are within a <body> block etc, have a look at the code in the AdminBarRemovalSniff which does the same for the <style> tag.
As for the code, please see my comments in-line. @timelsass For a first sniff, I can only say: my compliments, great effort!
I have to admit I haven't dug too deeply into the do/while loop as I'd already left so many comments, but I expect that code to be changed based on the comments anyhow, so I'll look at that more closely in the next review round.
Some general questions/issues:
- Does
$tagactually need to be a property ?
If you return the end token of an HTML tag, or even just the point when you stop looking, there is no real need to keep track of it in a property, it could just be a local variable within the function which starts atfalseany time the sniff gets passed a token.
In that case, you also wouldn't need to keep track of the$current_file. - Multi-line strings get one token per line without whitespace tokens between them, so in one or two places where you use
Token::$emptyTokens, you may not need to.
Other than that:
- Please add a
@sincetag to all sniff constants, properties and methods.
This will help keep track of what features where added when.
Note: For theUnitTest.phpfile, just a@sincetag in the class docblock is sufficient as those files will always have the same required methods, so those will always have been added at the time the sniff got added.
| * | ||
| * @var array | ||
| */ | ||
| public $tagsConfig = array( |
There was a problem hiding this comment.
@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 👍 .
| ), | ||
| ); | ||
|
|
||
| /** |
There was a problem hiding this comment.
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.
| use PHP_CodeSniffer\Util\Tokens; | ||
|
|
||
| /** | ||
| * Ensures functions are called within HTML tags. |
There was a problem hiding this comment.
| * Ensures functions are called within HTML tags. | |
| * Check that certain HTML tags contain the recommended WP functions to add attributes/attribute values. |
| public $supportedTokenizers = array( 'PHP' ); | ||
|
|
||
| /** | ||
| * Tag being searched. |
There was a problem hiding this comment.
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 ?
| * | ||
| * @return string Cleaned string. | ||
| */ | ||
| private function clean_str( $str ) { |
There was a problem hiding this comment.
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:
- Gather all the parts of the string first and concatenate them together.
- Then run the resulting string through the WPCS
strip_quotesfunction. - DON'T do this for text strings where no quotes are expected, such as
T_INLINE_HTML,T_HEREDOCorT_NOWDOC.
| // Required function not found. | ||
| if ( false === $foundFunction ) { | ||
| $phpcsFile->addError( | ||
| "Themes must call {$tagFn}() inside <{$tagName}> tags.", |
There was a problem hiding this comment.
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,
)
);| "RequiredFunction{$pascal}" | ||
| ); | ||
|
|
||
| return; |
There was a problem hiding this comment.
Having a return here effectively hides one error behind another which will make fixing things unwieldy, so it is better not to do this.
|
|
||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| > | ||
| ... | ||
| EOT; | ||
| ?> |
There was a problem hiding this comment.
- 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
classattribute 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.
| } | ||
|
|
||
| // Verify function( $param = 'optional' ) type. | ||
| if ( 'PHPCS_T_OPEN_PARENTHESIS' === $tokens[ $next ]['code'] ) { |
There was a problem hiding this comment.
| if ( 'PHPCS_T_OPEN_PARENTHESIS' === $tokens[ $next ]['code'] ) { | |
| if ( 'T_OPEN_PARENTHESIS' === $tokens[ $next ]['code'] ) { |
There was a problem hiding this comment.
Also: I'm not sure what you're trying to do here.
|
@timelsass did you have the time to look over the review? If you want, I could pull your changes and see if I can help with this sniff 🙂 |
|
I've changed the milestone to 0.2.x. Once you'll have more time I'd love for you to finish the sniff. If you need any help just let me or Juliette know and we'll help out any way we can 🙂 |
|
Hi, @timelsass can you just let us know if you wish to continue working on this sniff. If yes, can you rebase so that the checks on GH actions will run? |
Adds sniffs for
language_attributes()andbody_class()usage.language_attributes()requirement in: https://make.wordpress.org/themes/handbook/review/required/#codebody_class()requirement in: https://make.wordpress.org/themes/handbook/review/required/#templatesI think we should probably change the organization of the handbook, and add
language_attributes()to the Templates section to stay in line with the sniff, plus is a requirement of "templates".I know it's not 100% perfect as there can be some weird syntax and ways around with the mixes of various output methods in PHP + html, but the test cases were mostly the top common scenarios I saw, and have caught a couple of issues that made it past reviewers.
Example: https://themes.trac.wordpress.org/browser/nirmala/1.3.0/page-templates/blank.php#L23