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

phpcs: Fix unintentional exclusions #37122

Merged
merged 5 commits into from
Apr 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 23 additions & 0 deletions .github/files/lint-project-structure.sh
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,29 @@ for FILE in $(git -c core.quotepath=off ls-files 'projects/packages/**/block.jso
fi
done

# - In phpcs config, `<rule ref="Standard.Category.Sniff.Message"><severity>0</severity></rule>` doesn't do what you think.
debug "Checking for bad message exclusions in phpcs configs"
for FILE in $(git -c core.quotepath=off ls-files .phpcs.config.xml .phpcs.xml.dist .github/files/php-linting-phpcs.xml .github/files/phpcompatibility-dev-phpcs.xml '*/.phpcs.dir.xml' '*/.phpcs.dir.phpcompatibility.xml'); do
while IFS=$'\t' read -r LINE REF; do
EXIT=1
echo "::error file=$FILE,line=$LINE::PHPCS config attempts to set severity 0 for the sniff message \"$REF\". To exclude a single message from a sniff, use \`<rule ref=\"${REF%.*}\"><exclude name=\"$REF\"/></rule>\` instead."
done < <( php -- "$FILE" <<-'PHPDOC'
<?php
$doc = new DOMDocument();
$doc->load( $argv[1] );
$xpath = new DOMXPath( $doc );
function has_message( $v ) {
return count( explode(".", $v[0]->value) ) >= 4;
}
$xpath->registerNamespace("php", "http://php.net/xpath");
$xpath->registerPHPFunctions( "has_message" );
foreach ( $xpath->evaluate( "//rule[php:function(\"has_message\", @ref)][severity[normalize-space(.)=\"0\"]]" ) as $node ) {
echo "{$node->getLineNo()}\t{$node->getAttribute("ref")}\n";
}
PHPDOC
)
done

# - .nvmrc should match .github/versions.sh.
debug "Checking .nvmrc vs versions.sh"
if [[ "$(<.nvmrc)" != "$NODE_VERSION" ]]; then
Expand Down
5 changes: 5 additions & 0 deletions projects/packages/phpcs-filter/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,11 @@ You can still exclude such a rule by setting its severity to zero. The 4-compone
<!-- This works too. -->
<exclude name="Standard.Category.Rule.Message" />
</rule>

<!-- P.S. Don't do this, it doesn't work like you'd expect (it implicitly sets severity 0 for Standard.Category.Rule too). Use `<exclude>` as above for individual messages. -->
<rule ref="Standard.Category.Rule.Message">
<severity>0</severity>
</rule>
```

### `<file>`
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: added

Add a doc note warning against using `<severity>0</severity>` when excluding individual sniff messages.
6 changes: 4 additions & 2 deletions projects/packages/waf/.phpcs.dir.xml
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@
<!-- Some code here runs outside of WordPress code. -->
<!-- TODO: Split that by directory or something so we can do this only for the non-WordPress code. -->
<rule ref="Jetpack-NoWP" />
<rule ref="WordPress.PHP.DevelopmentFunctions.error_log_var_export">
<severity>0</severity>
<rule ref="WordPress.PHP.DevelopmentFunctions">
<exclude name="WordPress.PHP.DevelopmentFunctions.error_log_error_log"/>
<exclude name="WordPress.PHP.DevelopmentFunctions.error_log_var_dump"/>
<exclude name="WordPress.PHP.DevelopmentFunctions.error_log_var_export"/>
</rule>

</ruleset>
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: changed
Comment: Fix phpcs config. No change to functionality.


4 changes: 2 additions & 2 deletions projects/packages/wp-js-data-sync/.phan/baseline.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@
// PhanUndeclaredMethod : 3 occurrences
// PhanPluginDuplicateConditionalNullCoalescing : 2 occurrences
// PhanTypeMismatchArgumentProbablyReal : 2 occurrences
// PhanTypeMismatchReturn : 2 occurrences
// PhanImpossibleCondition : 1 occurrence
// PhanImpossibleTypeComparison : 1 occurrence
// PhanParamTooMany : 1 occurrence
// PhanPluginSimplifyExpressionBool : 1 occurrence
// PhanRedundantCondition : 1 occurrence
// PhanTypeMismatchArgumentNullable : 1 occurrence
// PhanTypeMismatchProperty : 1 occurrence
// PhanTypeMismatchReturn : 1 occurrence
// PhanUndeclaredTypeParameter : 1 occurrence
// PhanUnreferencedUseNormal : 1 occurrence

Expand All @@ -38,7 +38,7 @@
'src/schema/types/class-type-assoc-array.php' => ['PhanTypeMismatchArgumentNullable', 'PhanTypeMismatchReturn'],
'src/schema/types/class-type-string.php' => ['PhanImpossibleTypeComparison'],
'tests/php/schema/integration/test-integration-fallback-values.php' => ['PhanNonClassMethodCall'],
'tests/php/schema/integration/test-integration-parsing-errors.php' => ['PhanNonClassMethodCall', 'PhanParamTooFew', 'PhanTypeMismatchReturn'],
'tests/php/schema/integration/test-integration-parsing-errors.php' => ['PhanNonClassMethodCall', 'PhanParamTooFew'],
'tests/php/schema/type/test-type-assoc-array.php' => ['PhanTypeMismatchArgumentProbablyReal'],
],
// 'directory_suppressions' => ['src/directory_name' => ['PhanIssueName1', 'PhanIssueName2']] can be manually added if needed.
Expand Down
41 changes: 18 additions & 23 deletions projects/packages/wp-js-data-sync/.phpcs.dir.xml
Original file line number Diff line number Diff line change
Expand Up @@ -27,35 +27,30 @@
</properties>
</rule>

<rule ref="MediaWiki.Usage.ForbiddenFunctions.isset">
<severity>0</severity>
<rule ref="MediaWiki.Usage.ForbiddenFunctions">
<exclude name="MediaWiki.Usage.ForbiddenFunctions.isset"/>
</rule>

<rule ref="Squiz.Commenting.FunctionComment.MissingParamComment">
<severity>0</severity>
<rule ref="Squiz.Commenting.FunctionComment">
<exclude name="Squiz.Commenting.FunctionComment.MissingParamComment"/>
<exclude name="Squiz.Commenting.FunctionComment.MissingParamName"/>
<exclude name="Squiz.Commenting.FunctionComment.MissingParamTag"/>
<exclude name="Squiz.Commenting.FunctionComment.MissingReturn"/>
<exclude name="Squiz.Commenting.FunctionComment.ParamCommentFullStop"/>
</rule>
<rule ref="Squiz.Commenting.FunctionComment.MissingParamName">
<severity>0</severity>
<rule ref="Generic.Commenting.DocComment">
<exclude name="Generic.Commenting.DocComment.MissingShort"/>
</rule>
<rule ref="Squiz.Commenting.FunctionComment.MissingParamTag">
<severity>0</severity>
<rule ref="Squiz.Commenting.FunctionComment">
<exclude name="Squiz.Commenting.FunctionComment.Missing"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be nested in the above group (line 34).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤷 Phpcs doesn't seem to mind, and the original seemed to intentionally have all the "Missing" sniffs listed as a group.

</rule>
<rule ref="Squiz.Commenting.FunctionComment.MissingReturn">
<severity>0</severity>
<rule ref="Squiz.Commenting.ClassComment">
<exclude name="Squiz.Commenting.ClassComment.Missing"/>
</rule>
<rule ref="Generic.Commenting.DocComment.MissingShort">
<severity>0</severity>
<rule ref="Squiz.Commenting.FileComment">
<exclude name="Squiz.Commenting.FileComment.Missing"/>
</rule>
<rule ref="Squiz.Commenting.FunctionComment.Missing">
<severity>0</severity>
</rule>
<rule ref="Squiz.Commenting.ClassComment.Missing">
<severity>0</severity>
</rule>
<rule ref="Squiz.Commenting.FileComment.Missing">
<severity>0</severity>
</rule>
<rule ref="Squiz.Commenting.VariableComment.Missing">
<severity>0</severity>
<rule ref="Squiz.Commenting.VariableComment">
<exclude name="Squiz.Commenting.VariableComment.Missing"/>
</rule>
</ruleset>
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: fixed

Fix phpcs config, and phpdoc comments that were easy to fix.
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ final class Data_Sync_Entry_Adapter implements Data_Sync_Entry {

/**
* For more explanation, see the class docblock.
*
* @see Data_Sync_Entry_Adapter
* The constructor accepts any entry that subscribes to at least "Entry_Can_Get", but can also
* subscribe to any of the other Entry_Can_* interfaces.
Expand Down
4 changes: 2 additions & 2 deletions projects/packages/wp-js-data-sync/src/class-data-sync.php
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,8 @@ public function get_registry() {
* However, you can provide an `$entry` instance that subscribes Entry_Can_* methods.
* If you do, `Entry_Can_Get` interface is required, and all other Entry_Can_* interfaces are optional.
*
* @param string $key - The key to register the entry under.
* @param Parser $parser - The parser to use for the entry.
* @param string $key - The key to register the entry under.
* @param Parser $parser - The parser to use for the entry.
* @param Entry_Can_Get $custom_entry_instance - The entry to register. If null, a new Data_Sync_Option will be created.
*
* @return void
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ interface Data_Sync_Action {
/**
* Handles the action logic.
*
* @param mixed $data JSON Data passed to the action.
* @param mixed $data JSON Data passed to the action.
* @param \WP_REST_Request $request The request object.
* @return mixed
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ class Action_Endpoint {
/**
* This class handles endpoints for DataSync actions.
*
*
* @param $namespace
* @param $key
* @param $action_name
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,32 +97,36 @@ public function register_rest_routes() {
}
}

/*
/**
* Handle GET Requests on /wp-json/<namespace>/<route>
*
* @param \WP_REST_Request $request - The request object.
*/
public function handle_get( $request ) {
return $this->handler( $request, 'get' );
}

/*
/**
* Handle POST, PUT, PATCH Requests on /wp-json/<namespace>/<route>/set
*
* @param \WP_REST_Request $request - The request object.
*/
public function handle_set( $request ) {
return $this->handler( $request, 'set' );
}

/*
/**
* Handle POST, PUT, PATCH Requests on /wp-json/<namespace>/<route>/merge
*
* @param \WP_REST_Request $request - The request object.
*/
public function handle_merge( $request ) {
return $this->handler( $request, 'merge' );
}

/*
/**
* Handle POST, DELETE Requests on /wp-json/<namespace>/<route>/delete
*
* @param \WP_REST_Request $request - The request object.
*/
public function handle_delete( $request ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,8 @@ public function __toString() {
*
* @param Parser $parser
*
*
* @return $this
* @see Schema::either()
*
*/
private function or( Parser $parser ) {
if ( $this->parser instanceof Modifier_Fallback ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@
* ];
*
* $parsed_data = $my_schema->parse($input_data);
*
*/
class Schema {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,24 @@ interface Parser extends \JsonSerializable {
* If the input value is invalid, the method should return a default value.
* or throw an exception, depending on the implementation.
*
* @param mixed $value The input value to be parsed.
* @param Schema_Context $context Schema validation metadata.
* @param mixed $value The input value to be parsed.
* @param Schema_Context $context Schema validation metadata.
*
* @return mixed The parsed value.
* @throws \RuntimeException If the input value is invalid.
*
*/
public function parse( $value, $context );

/**
* The describe method is responsible for returning a description of the schema.
*
* @return array
*/
public function schema();

/**
* The __toString method is responsible for returning a string representation of the schema.
*
* @return string
*/
public function __toString();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Automattic\Jetpack\WP_JS_Data_Sync\Schema\Types;

use Automattic\Jetpack\WP_JS_Data_Sync\Schema\Parser;
use Automattic\Jetpack\WP_JS_Data_Sync\Schema\Schema_Context;
use Automattic\Jetpack\WP_JS_Data_Sync\Schema\Schema_Error;

class Type_Array implements Parser {
Expand All @@ -18,13 +19,15 @@ public function __construct( Parser $parser ) {
$this->parser = $parser;
}

/*
/**
* This parse method expects that the $data passed to it is
* an array of other Parser instances.
*
* @param $data - an array of something to be parsed.
* @param array $value - an array of something to be parsed.
* @param Schema_Context $context - Schema context.
*
* @return array
* @throws Schema_Error If $value is not an array.
*/
public function parse( $value, $context ) {
if ( ! is_array( $value ) ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ public function __construct( $assoc_parser_array ) {
*
* @return array
* @throws Schema_Error - If the $data passed to it is not an associative array.
*
*/
public function parse( $value, $context ) {
// Allow coercing stdClass objects (often returned from json_decode) to an assoc array.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,11 @@ private function get_assoc_schema( $schema, $levels = 3, $i = 1 ) {
}

/**
* Creates an associative array with nested levels containing 'hello world' string.
* Creates an associative array with nested levels containing $data.
*
* @param int $levels The depth of nesting in the associative array.
* @param mixed $data Data to contain.
* @param int $levels The depth of nesting in the associative array.
* @param int $i Current nesting depth, used for recursive calls.
*
* @return array The associative array with data.
*/
Expand Down
33 changes: 18 additions & 15 deletions projects/plugins/boost/.phpcs.dir.xml
Original file line number Diff line number Diff line change
Expand Up @@ -21,27 +21,30 @@
</properties>
</rule>

<rule ref="WordPress.Files.FileName.InvalidClassFileName">
<severity>0</severity>
<rule ref="WordPress.Files.FileName">
<exclude name="WordPress.Files.FileName.InvalidClassFileName"/>
<exclude name="WordPress.Files.FileName.NotHyphenatedLowercase"/>
</rule>

<rule ref="Squiz.Commenting.FileComment.Missing">
<severity>0</severity>
<rule ref="Squiz.Commenting.FileComment">
<exclude name="Squiz.Commenting.FileComment.Missing"/>
<exclude name="Squiz.Commenting.FileComment.MissingPackageTag"/>
<exclude name="Squiz.Commenting.FileComment.WrongStyle"/>
</rule>
<rule ref="Squiz.Commenting.ClassComment.Missing">
<severity>0</severity>
<rule ref="Squiz.Commenting.ClassComment">
<exclude name="Squiz.Commenting.ClassComment.Missing"/>
</rule>
<rule ref="Squiz.Commenting.FunctionComment.Missing">
<severity>0</severity>
<rule ref="Squiz.Commenting.FunctionComment">
<exclude name="Squiz.Commenting.FunctionComment.Missing"/>
<exclude name="Squiz.Commenting.FunctionComment.MissingParamTag"/>
<exclude name="Squiz.Commenting.FunctionComment.ParamCommentFullStop"/>
<exclude name="Squiz.Commenting.FunctionComment.MissingParamComment"/>
anomiex marked this conversation as resolved.
Show resolved Hide resolved
</rule>
<rule ref="Squiz.Commenting.FunctionComment.MissingParamComment">
<severity>0</severity>
<rule ref="Generic.Commenting.DocComment">
<exclude name="Generic.Commenting.DocComment.MissingShort"/>
</rule>
<rule ref="Generic.Commenting.DocComment.MissingShort">
<severity>0</severity>
</rule>
<rule ref="Squiz.Commenting.VariableComment.Missing">
<severity>0</severity>
<rule ref="Squiz.Commenting.VariableComment">
<exclude name="Squiz.Commenting.VariableComment.Missing"/>
</rule>

<rule ref="VariableAnalysis.CodeAnalysis.VariableAnalysis">
Expand Down
1 change: 0 additions & 1 deletion projects/plugins/boost/app/class-jetpack-boost.php
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ private function register_deactivation_hook() {
* @param array $allowed_query_args The list of allowed query args.
*
* @return array The modified list of allowed query args.

*/
public static function whitelist_query_args( $allowed_query_args ) {
$allowed_query_args[] = Generator::GENERATE_QUERY_ACTION;
Expand Down
1 change: 1 addition & 0 deletions projects/plugins/boost/app/contracts/Pluggable.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ interface Pluggable extends Has_Setup, Has_Slug {
/**
* Whether the feature is available for use.
* Use this to check for feature flags, etc.
*
* @return bool
*/
public static function is_available();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* Boost Setup Prompt
* DEPRECATED - this prompt has been removed as of version 2.3.1
*/

?>
<div class="jb-setup-banner">
<div class="jb-setup-banner__content-top-text">
Expand Down
Loading
Loading