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.

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

Add ValidTaxonomySlugSniff #2485

Draft
wants to merge 23 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
cdbb5c0
Add ValidTaxonomySlugSniff
IanDelMar Sep 10, 2024
9b101b2
Fix return tag
IanDelMar Sep 10, 2024
a5653a3
Update WordPress/AbstractValidSlugSniff.php
IanDelMar Sep 10, 2024
dcddda1
Update WordPress-Extra/ruleset.xml
IanDelMar Sep 10, 2024
265d46f
Update WordPress/Sniffs/NamingConventions/ValidTaxonomySlugSniff.php
IanDelMar Sep 10, 2024
5b7267e
Update WordPress/AbstractValidSlugSniff.php
IanDelMar Sep 10, 2024
4a1b3d0
Update WordPress/Docs/NamingConventions/ValidTaxonomySlugStandard.xml
IanDelMar Sep 10, 2024
dc545ee
Update WordPress/Docs/NamingConventions/ValidTaxonomySlugStandard.xml
IanDelMar Sep 10, 2024
2150d29
Update WordPress/Sniffs/NamingConventions/ValidPostTypeSlugSniff.php
IanDelMar Sep 10, 2024
b829a79
Update WordPress/Helpers/WPReservedKeywordHelper.php
IanDelMar Sep 10, 2024
ce18b1c
Rename WPReservedKeywordHelper
IanDelMar Sep 10, 2024
ad1a096
Merge branch 'taxonomy-name' of https://github.com/IanDelMar/WordPres…
IanDelMar Sep 10, 2024
04922b8
Update WordPress/AbstractValidSlugSniff.php
IanDelMar Sep 10, 2024
5ec64ac
Rename WPReservedNamesHelper file
IanDelMar Sep 10, 2024
4e6f7f7
Use name instead of keyword
IanDelMar Sep 10, 2024
7b578cf
Merge branch 'taxonomy-name' of https://github.com/IanDelMar/WordPres…
IanDelMar Sep 10, 2024
c8dbf93
Use private properties
IanDelMar Sep 10, 2024
46b5117
Add "last updated" indicator
IanDelMar Sep 10, 2024
ad05b95
Add since tag
IanDelMar Sep 11, 2024
dc17ea7
Move property setup into register()
IanDelMar Sep 11, 2024
d9e0ee8
Move max length info to concrete classes
IanDelMar Sep 11, 2024
05f42ca
Fix data order
IanDelMar Sep 11, 2024
2b2b097
Merge branch 'taxonomy-name' of https://github.com/IanDelMar/WordPres…
IanDelMar Sep 11, 2024
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
3 changes: 2 additions & 1 deletion .github/release-checklist.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ PR for tracking changes for the x.x.x release. Target release date: **DOW MONTH
- [ ] `$pluggable_functions` in `WordPress.NamingConventions.PrefixAllGlobals` - PR #xxx
- [ ] `$pluggable_classes` in `WordPress.NamingConventions.PrefixAllGlobals` - PR #xxx
- [ ] `$target_functions` in `WordPress.Security.PluginMenuSlug` - PR #xxx
- [ ] `$reserved_names` in `WordPress.NamingConventions.ValidPostTypeSlug` - PR #xxx
- [ ] `$post_types` in `WPReservedKeywordHelper` - PR #xxx
- [ ] `$terms` in `WPReservedKeywordHelper` - PR #xxx
- [ ] `$wp_time_constants` in `WordPress.WP.CronInterval` - PR #xxx
- [ ] `$known_test_classes` in `IsUnitTestTrait` - PR #xxx
- [ ] ...etc...
Expand Down
3 changes: 3 additions & 0 deletions WordPress-Extra/ruleset.xml
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,9 @@
<!-- Validates post type slugs for valid characters, length and reserved keywords. -->
<rule ref="WordPress.NamingConventions.ValidPostTypeSlug"/>

<!-- Validates post type slugs for valid characters, length and reserved keywords. -->
IanDelMar marked this conversation as resolved.
Show resolved Hide resolved
<rule ref="WordPress.NamingConventions.ValidTaxonomySlug"/>

<!-- https://github.com/WordPress/WordPress-Coding-Standards/issues/1157 -->
<rule ref="WordPress.Security.PluginMenuSlug"/>
<rule ref="WordPress.WP.CronInterval"/>
Expand Down
276 changes: 276 additions & 0 deletions WordPress/AbstractValidSlugSniff.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,276 @@
<?php
/**
* WordPress Coding Standard.
*
* @package WPCS\WordPressCodingStandards
* @link https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards
* @license https://opensource.org/licenses/MIT MITnamespace WordPressCS\WordPress;
IanDelMar marked this conversation as resolved.
Show resolved Hide resolved
*/

namespace WordPressCS\WordPress;

use PHP_CodeSniffer\Util\Tokens;
use PHPCSUtils\Tokens\Collections;
use PHPCSUtils\Utils\PassedParameters;
use PHPCSUtils\Utils\TextStrings;
use WordPressCS\WordPress\AbstractFunctionParameterSniff;

/**
* Validates names passed to a function call.
*
* Checks slugs for the presence of invalid characters, excessive length,
* and reserved keywords.
*/
IanDelMar marked this conversation as resolved.
Show resolved Hide resolved
abstract class AbstractValidSlugSniff extends AbstractFunctionParameterSniff {

/**
* Slug type. E.g. 'post type' for a post type slug.
*
* @var string
*/
protected $slug_type;

/**
* Plural of the slug type. E.g. 'post types' for a post type slug.
*
* @var string
*/
protected $slug_type_plural;

/**
* Max length of a slug is limited by the SQL field.
*
* @var int
*/
protected $max_length;

/**
* Regex to validate the characters that can be used as the slug.
*
* @var string
*/
protected $valid_characters;
IanDelMar marked this conversation as resolved.
Show resolved Hide resolved

/**
* Array of reserved names which can not be used by themes and plugins.
IanDelMar marked this conversation as resolved.
Show resolved Hide resolved
*
* @var array<string, true> Key is reserved name, value irrelevant.
*/
protected $reserved_names;

/**
* All valid tokens for the slug parameter.
*
* Set in `register()`.
*
* @var array<int|string, int|string>
*/
private $valid_tokens = array();

/**
* Constructor.
*
* @since 3.0.0
*/
public function __construct() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not keen on this "abstract methods to properties" setting of sniff specific information.

I'm not even sure this abstract is a good idea to begin with.

Abstract sniffs should generally not throw errors. That should be done in the actual sniffs themselves. Doing so in the abstract greatly reduces the re-usability of any such abstract (as the error may be needed for slug type A, but not for slug type B or the error may need to be worded slightly differently as the effect of the issue is different for a certain slug type etc etc).

And if you take out the errors and just keep the logic, there is barely anything left of the abstract.

The only acceptable way to throw errors from an abstract is via a separate method which can be overloaded in the concrete sniff.

Along the same lines, the abstract methods currently defined in this sniff may not apply for all slug types, so again, this reduces the re-usability.

Considering the original sniff already used class constants (for constant information) and removing those is no-go (breaking change), it could be considered to use class constants (and maybe the odd non-abstract method) as "feature toggles" + for the actual information.

I'm thinking along the lines of logic like this (pseudo code):

if (defined('static::MAX_LENGTH')) {
    // Execute the max length check.
}

Having said that, I'm not convinced of the value of the abstract sniff for something this specific as checking slug name rules.

$this->target_functions = $this->get_target_functions();
$this->slug_type = $this->get_slug_type();
$this->slug_type_plural = $this->get_slug_type_plural();
$this->valid_characters = $this->get_valid_characters();
$this->max_length = $this->get_max_length();
$this->reserved_names = $this->get_reserved_names();
}

/**
* Retrieve function and parameter(s) pairs this sniff is looking for.
*
* The parameter or an array of parameters keyed by target function.
* An array of names is supported to allow for functions for which the
* parameter names have undergone name changes over time.
*
* @return array<string, string|array<string>> Function parameter(s) pairs.
*/
abstract protected function get_target_functions();

/**
* Retrieve the slug type.
*
* @return string The slug type.
*/
abstract protected function get_slug_type();

/**
* Retrieve the plural slug type.
*
* @return string The plural slug type.
*/
abstract protected function get_slug_type_plural();

/**
* Retrieve the regex to validate the characters that can be used as
* the slug.
*
* @return string Regular expression.
*/
abstract protected function get_valid_characters();

/**
* Retrieve the max length of a slug.
*
* The length is limited by the SQL field.
IanDelMar marked this conversation as resolved.
Show resolved Hide resolved
*
* @return int The maximum length of a slug.
*/
abstract protected function get_max_length();

/**
* Retrieve the reserved names which can not be used by themes and plugins.
*
* @return array<string, true> Key is reserved name, value irrelevant.
*/
abstract protected function get_reserved_names();

/**
* Returns an array of tokens this test wants to listen for.
*
* @return array
*/
public function register() {
$this->valid_tokens = Tokens::$textStringTokens + Tokens::$heredocTokens + Tokens::$emptyTokens;
IanDelMar marked this conversation as resolved.
Show resolved Hide resolved
return parent::register();
}

/**
* Process the parameter of a matched function.
*
* Errors on invalid names when reserved keywords are used,
IanDelMar marked this conversation as resolved.
Show resolved Hide resolved
* the name is too long, or contains invalid characters.
*
* @param int $stackPtr The position of the current token in the stack.
* @param string $group_name The name of the group which was matched.
* @param string $matched_content The token content (function name) which was matched
* in lowercase.
* @param array $parameters Array with information about the parameters.
*
* @return void
*/
public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) {
$slug_param = PassedParameters::getParameterFromStack(
$parameters,
1,
Copy link
Member

Choose a reason for hiding this comment

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

Please don't hard-code the parameter position in an abstract sniff. This reduces re-usability. This information should come from the concrete sniff.

Copy link
Author

Choose a reason for hiding this comment

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

The parameter name must be provided by the concrete sniff. If that's the case, the offset is superseded unless the parameter name is incorrect. Am I understanding this correctly?

Copy link
Member

@jrfnl jrfnl Sep 10, 2024

Choose a reason for hiding this comment

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

The parameter name must be provided by the concrete sniff. If that's the case, the offset is superseded unless the parameter name is incorrect. Am I understanding this correctly?

You are totally misunderstanding this.

In PHP, parameters can be passed positionally, i.e. my_function($first, $second, $third) or by name, i.e. my_function($first, third: $value).

To get the correct parameter from a function call and handle both ways of passing parameters, the PassedParameters::getParameterFromStack() method needs to know both the parameter name(s) and the parameter position.

And as this is an abstract sniff, the sniff cannot rely on the "slug"-like parameter always being in the first position.

Copy link
Author

Choose a reason for hiding this comment

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

I see! I thought this was checking against the name in the function declaration, rather than checking for named parameters. My mistake. If I had been more attentive, I would have noticed the line, 'First check for a named parameter.' As you may have noticed, I'm not very familiar with the WPCS or the PHPCS codebase. 😅 😇

Copy link
Member

Choose a reason for hiding this comment

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

PHPCS has no access to the function declaration (unless the function is declared in the same file). For this type of code analysis, you only look at the function call.

$this->target_functions[ $matched_content ]
);
if ( false === $slug_param || '' === $slug_param['clean'] ) {
// Error for using empty slug.
$this->phpcsFile->addError(
'%s() called without a %s slug. The slug must be a non-empty string.',
false === $slug_param ? $stackPtr : $slug_param['start'],
'Empty',
array(
$matched_content,
$this->slug_type,
)
);
return;
}

$string_start = $this->phpcsFile->findNext( Collections::textStringStartTokens(), $slug_param['start'], ( $slug_param['end'] + 1 ) );
$string_pos = $this->phpcsFile->findNext( Tokens::$textStringTokens, $slug_param['start'], ( $slug_param['end'] + 1 ) );

$has_invalid_tokens = $this->phpcsFile->findNext( $this->valid_tokens, $slug_param['start'], ( $slug_param['end'] + 1 ), true );
if ( false !== $has_invalid_tokens || false === $string_pos ) {
// Check for non string based slug parameter (we cannot determine if this is valid).
$this->phpcsFile->addWarning(
'The %s slug is not a string literal. It is not possible to automatically determine the validity of this slug. Found: %s.',
$stackPtr,
'NotStringLiteral',
array(
$this->slug_type,
$slug_param['clean'],
),
3
);
return;
}

$slug = TextStrings::getCompleteTextString( $this->phpcsFile, $string_start );
if ( isset( Tokens::$heredocTokens[ $this->tokens[ $string_start ]['code'] ] ) ) {
// Trim off potential indentation from PHP 7.3 flexible heredoc/nowdoc content.
$slug = ltrim( $slug );
}

// Warn for dynamic parts in the slug parameter.
if ( 'T_DOUBLE_QUOTED_STRING' === $this->tokens[ $string_pos ]['type']
|| ( 'T_HEREDOC' === $this->tokens[ $string_pos ]['type']
&& strpos( $this->tokens[ $string_pos ]['content'], '$' ) !== false )
) {
$this->phpcsFile->addWarning(
'The %s slug may, or may not, get too long with dynamic contents and could contain invalid characters. Found: "%s".',
$string_pos,
'PartiallyDynamic',
array(
$this->slug_type,
$slug,
)
);
$slug = TextStrings::stripEmbeds( $slug );
}

if ( preg_match( $this->valid_characters, $slug ) === 0 ) {
// Error for invalid characters.
$this->phpcsFile->addError(
'%s() called with invalid %s "%s". %s contains invalid characters. Only lowercase alphanumeric characters, dashes, and underscores are allowed.',
$string_pos,
'InvalidCharacters',
array(
$matched_content,
$this->slug_type,
ucfirst( $this->slug_type ),
$slug,
IanDelMar marked this conversation as resolved.
Show resolved Hide resolved
)
);
}

if ( isset( $this->reserved_names[ $slug ] ) ) {
// Error for using reserved slug names.
$this->phpcsFile->addError(
'%s() called with reserved %s "%s". Reserved %s should not be used as they interfere with the functioning of WordPress itself.',
$string_pos,
'Reserved',
array(
$matched_content,
$this->slug_type,
$slug,
$this->slug_type_plural,
)
);
} elseif ( stripos( $slug, 'wp_' ) === 0 ) {
// Error for using reserved slug prefix.
$this->phpcsFile->addError(
'The %s passed to %s() uses a prefix reserved for WordPress itself. Found: "%s".',
$string_pos,
'ReservedPrefix',
array(
$this->slug_type,
$matched_content,
$slug,
)
);
}

// Error for slugs that are too long.
if ( strlen( $slug ) > $this->max_length ) {
$this->phpcsFile->addError(
'A %s slug must not exceed %d characters. Found: "%s" (%d characters).',
$string_pos,
'TooLong',
array(
$this->slug_type,
$this->max_length,
$slug,
strlen( $slug ),
)
);
}
}
}
Loading
Loading