Adding documentation to WordPress.WP.DiscouragedConstants #2493
Conversation
There was a problem hiding this comment.
Thanks for creating this PR, and sorry for the delay in reviewing it, @RafaelFunchal! I left some comments with points I believe need to be addressed before it can be merged.
| <em>define("TEMPLATEPATH", "foo");</em> | ||
| <img src=" | ||
| <?php echo <em>$foo</em>; ?> | ||
| /img/logo.svg" /> |
There was a problem hiding this comment.
I don't think it is necessary to include the <img> tag here. Including it means that this code example will trigger both sniff warnings and not just the one associated with this <standard> block. If you decide to keep the <img>, there are some fixes that need to be made. This code currently generates a syntax error due to the missing PHP closing tag after the call to define(). Also, I believe it should echo TEMPLATEPATH instead of the undefined variable $foo.
| </code> | ||
| <code title="Invalid: The TEMPLATEPATH constant is set by a theme."> | ||
| <![CDATA[ | ||
| <em>define("TEMPLATEPATH", "foo");</em> |
There was a problem hiding this comment.
Maybe it is better to highlight just the constant name instead of the whole line as it is not any call to define() that triggers this sniff?
| <code_comparison> | ||
| <code title="Valid: A function is used in the code to retrieve the theme directory."> | ||
| <![CDATA[ | ||
| <img src=" |
There was a problem hiding this comment.
AFAIK, it is not common to indent an HTML tag like it is done here. Maybe instead of fixing the indentation, we can simplify the example and make it easier to understand what this sniff is about? I believe the code sample could be trimmed down to echo <em>TEMPLATEPATH</em>;.
There was a problem hiding this comment.
Actually just echo <em>TEMPLATEPATH</em>; is not a realistic example. Maybe something like the example below is better?
$subdir = TEMPLATEPATH . '/subdir/';And something similar for the example with get_template_directory_uri().
| > | ||
| <standard> | ||
| <![CDATA[ | ||
| The usage of WordPress Constants is discouraged. |
There was a problem hiding this comment.
| The usage of WordPress Constants is discouraged. | |
| The usage of WordPress constants is discouraged. |
| </code_comparison> | ||
| <standard> | ||
| <![CDATA[ | ||
| Defining WordPress Constants is discouraged. |
There was a problem hiding this comment.
| Defining WordPress Constants is discouraged. | |
| Defining WordPress constants is discouraged. |
| > | ||
| <standard> | ||
| <![CDATA[ | ||
| The usage of WordPress Constants is discouraged. |
There was a problem hiding this comment.
I wonder if it makes sense to list here and in the other <standard> block the discouraged constants, as this sniff triggers a warning only for a specific set of WP constants and not all constants. What do you think?
There was a problem hiding this comment.
On a second thought, maybe providing the whole list is too much, we could simply say something like The use of certain WordPress constants is discouraged.
| ]]> | ||
| </standard> | ||
| <code_comparison> | ||
| <code title="Valid: A function is used in the code to retrieve the theme directory."> |
There was a problem hiding this comment.
This is not explicitly mentioned in #1722, but my understanding, based on checking the documentation from other sniffs, is that the description of the valid and invalid examples should be generic and describe what constitutes a valid example for this particular check that the sniff performs instead of describing the code sample provided.
Maybe here, the valid description should mention that the code does not use one of the discouraged WP constants or something like that?
| </code> | ||
| <code title="Invalid: The TEMPLATEPATH constant is set by a theme."> | ||
| <
Related to #1722