Enforce @since tags in /packages/block-serialization-default-parser/ and other files #60007

anton-vlasenko · 2024-03-19T23:46:01Z

What?

This PR enhances support for checking @since tags within various code elements such as class, trait, interface, enum declarations, properties, and method declarations. Additionally, it extends support to check @since tags for WordPress functions that invoke hooks like 'do_action', 'do_action_ref_array', 'apply_filters', and 'apply_filters_ref_array'.

The sniff only checks /** style comments.
The sniffs supports multiple @since tags per token.

Fixes #59826.

Why?

This PR improves the overall code adherence to WordPress coding standards. By enforcing @since tags, it ensures better documentation and version tracking, making it easier for developers to understand the evolution of the codebase over time.

How?

Added support for checking @since tags for class, trait, interface, and enum declarations.
Added support for checking @since tags for class and trait properties;
Added support for checking @since tags for class, interface, and trait method declarations;
Added support for checking @since tags for WordPress functions that invoke hooks: do_action(), do_action_ref_array(), apply_filters(), apply_filters_ref_array().
Refactored unit tests; duplicate code moved to the new GutenbergCS\Gutenberg\Tests\AbstractSniffUnitTest class.

Testing Instructions

Define a class, trait, interface, and enum without a @since tag and ensure that the linter checks fail.
Define a function, class method, interface method, and trait method without a @since tag and ensure that the linter checks fail. Ensure that the sniff skips checking private methods.
Attempt to use one of the WordPress functions that invoke hooks such as do_action(), do_action_ref_array(), apply_filters(), apply_filters_ref_array() without specifying a @since tag. Confirm that the linter checks fail.
Define a class property or trait property without a @since tag and ensure that the linter checks fail.

Testing Instructions for Keyboard

Screenshots or screencast

github-actions · 2024-03-27T21:50:44Z

Flaky tests detected in d7f8794.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8723082284
📝 Reported issues:

[Flaky Test] should multi-select block with text selection and a block without text selection #59579 in /test/e2e/specs/editor/various/multi-block-selection.spec.js

github-actions · 2024-04-03T13:58:37Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: anton-vlasenko <antonvlasenko@git.wordpress.org>
Co-authored-by: rodrigoprimo <rodrigosprimo@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

anton-vlasenko · 2024-04-03T19:53:55Z

Should the sniff skip checking private class methods and properties for the existence of the @since tag?
I assume private class methods and properties are not available to plugin authors since they cannot be inherited by the classes that extend WordPress Core classes.
Unfortunately, I couldn't find that information in the WordPress coding standards (https://developer.wordpress.org/coding-standards/inline-documentation-standards/php/).
@azaozz, maybe you can provide some advice on that?

Update: I will update the pull request to include checking private class properties and methods (as advised by @azaozz).

rodrigoprimo

Good job, @anton-vlasenko! I left some comments that occurred to me while checking the file changes. I focused on the changes to the sniff code. As you know, I'm not super familiar with the Gutenberg project, so some of my comments might not apply.

rodrigoprimo · 2024-04-05T14:58:46Z

.../php/gutenberg-coding-standards/Gutenberg/Sniffs/Commenting/FunctionCommentSinceTagSniff.php

@@ -12,28 +12,71 @@
 use PHP_CodeSniffer\Files\File;
 use PHP_CodeSniffer\Sniffs\Sniff;
 use PHP_CodeSniffer\Util\Tokens;
+use PHPCSUtils\Tokens\Collections;


Nice that you are using PHPCSUtils in this sniff!

Yes, thank you for introducing me to PHPCSUtils.

rodrigoprimo · 2024-04-05T15:01:28Z

.../php/gutenberg-coding-standards/Gutenberg/Sniffs/Commenting/FunctionCommentSinceTagSniff.php

+		// The content of the current token.
+		$hook_function = $tokens[ $stack_pointer ]['content'];
+
+		$hook_invocation_functions = array(


Should do_action_deprecated() and apply_filters_deprecated() be supported as well? I'm not sure if the @since tag is required when calling those functions or not, but I see it is used in some calls that I randomly checked in the WP repository. I did not find any calls to those functions in the GB repository.

I removed support for these deprecated functions at one point, but I agree, it's better to have these hooks covered. Let's add them back. Fixed in aed56d2

I remember using them in the Gutenberg plugin at some point, so it's great idea to cover them 👍🏻

Thanks for adding them back!

rodrigoprimo · 2024-04-05T15:02:29Z

.../php/gutenberg-coding-standards/Gutenberg/Sniffs/Commenting/FunctionCommentSinceTagSniff.php

+	 * @param int  $stack_pointer The position (stack pointer) in the token stack from which to start searching backwards.
+	 * @return array|false An array with the starting and ending token positions of the found docblock, or false if no docblock is found.
+	 */
+	protected static function find_docblock( File $phpcs_file, $stack_pointer ) {


Outside of the scope of this PR, but I wonder if it would make sense to start a conversation about adding find_docblock() to PHPCSUtils?

This could be done, but let me note that its implementation was largely copied from PHP_CodeSniffer\Standards\PEAR\Sniffs\Commenting\FunctionCommentSniff::process(), so it is not new code.

rodrigoprimo · 2024-04-05T15:06:16Z

.../php/gutenberg-coding-standards/Gutenberg/Sniffs/Commenting/FunctionCommentSinceTagSniff.php

+	 * @param File $phpcs_file    The file being scanned.
+	 * @param int  $stack_pointer The position of the hook token in the stack.
+	 */
+	protected function process_hook( File $phpcs_file, $stack_pointer ) {


The code below triggers a false positive MissingHookSinceTag error (there might be other false positive cases if there are other cases where the name of one of the hook functions can be used as a string and not in a function call):

class do_action { }

This is likely not a problem for this sniff in the context of the Gutenberg project, but something to be aware of if we move this sniff to WPCS in the future. Not sure if you want to address it now or not.

Thanks for addressing this as well. The is_function_call() method seems to be working correctly as far as I can check for the purposes needed for this sniff.

rodrigoprimo · 2024-04-05T15:09:58Z

.../php/gutenberg-coding-standards/Gutenberg/Sniffs/Commenting/FunctionCommentSinceTagSniff.php

+		list( $doc_block_start_token, $doc_block_end_token ) = $docblock;
+
+		$version_token = static::parse_since_tags( $phpcs_file, $doc_block_start_token, $doc_block_end_token );
+		if ( false === $version_token ) {


Is there a scenario I'm missing where $version_token is false?

If there is, maybe calling FunctionCommentSinceTagSniff::parse_since_tags() and GetTokensAsString::compact() twice inside this method is unnecessary? I haven't spent too much time checking the code, but it is not immediately clear to me the difference between the two code blocks where those two functions are called.

Good catch! This block of code wasn't needed and was added by mistake. Removed in 7a0bd41.

rodrigoprimo · 2024-04-05T15:11:22Z

.../php/gutenberg-coding-standards/Gutenberg/Sniffs/Commenting/FunctionCommentSinceTagSniff.php

+
+		foreach ( $version_tags as $since_tag_token => $version_value_token ) {
+			if ( null === $version_value_token ) {
+				$phpcs_file->addError( $missing_since_tag_error_message, $since_tag_token, $violation_code );


Maybe the error message here could be different as the @since tag is not missing and what is missing is its value?

rodrigoprimo · 2024-04-05T15:12:53Z

.../php/gutenberg-coding-standards/Gutenberg/Sniffs/Commenting/FunctionCommentSinceTagSniff.php

-			$phpcsFile->addError( $missing_since_tag_error_message, $stackPtr, 'MissingSinceTag' );
+		list( $doc_block_start_token, $doc_block_end_token ) = $docblock;
+
+		$version_tags = static::parse_since_tags( $phpcs_file, $doc_block_start_token, $doc_block_end_token );


This logic to get the @since tags and then check the version values seems to be repeated a few times across this class. Maybe it can be consolidated in a single method? Maybe even the logic above to find the docblock and return a error if one is not found can also be consolidated?

I agree. I've refactored the code to avoid duplicates.

rodrigoprimo · 2024-04-05T15:17:55Z

...hp/gutenberg-coding-standards/Gutenberg/Tests/Commenting/FunctionCommentSinceTagUnitTest.inc

+function qux() {
+}
+
+$variable = 'some_value'; /** Some comment @since 123 */


I'm failing to see the value of some tests. For example, I believe those two cases that I highlighted were already covered on lines 2 and 24. The same applies to the calls to the hook functions inside the foobar method of a class and an abstract class. There might be other cases that I didn't check. Are those tests testing different things that I'm not seeing or are they duplicates?

In my view, the code on lines 24 and 60 differs because it conducts the test in different contexts: the first instance occurs within the function body, while the latter does not. My goal was to accommodate a wide range of use cases.

Similarly, the foobar method is implemented across various structures such as traits, abstract classes, and classes to ensure comprehensive coverage of potential use cases.

rodrigoprimo · 2024-04-05T15:33:50Z

.../php/gutenberg-coding-standards/Gutenberg/Sniffs/Commenting/FunctionCommentSinceTagSniff.php

+		$violation_code = 'MissingPropertySinceTag';
+
+		$scope_modifier = Variables::getMemberProperties( $phpcs_file, $stack_pointer )['scope'];
+		if ( ( $scope_modifier === 'protected'


This check is very similar to a check in process_function_token(). Does it make sense to consolidate both in a helper method?

Fixed in febac6f

rodrigoprimo · 2024-04-05T15:39:08Z

test/php/gutenberg-coding-standards/Gutenberg/ruleset.xml

@@ -6,5 +6,6 @@
 	<rule ref="Gutenberg.CodeAnalysis.GuardedFunctionAndClassNames"/>
 	<rule ref="Gutenberg.CodeAnalysis.ForbiddenFunctionsAndClasses"/>
 	<rule ref="Gutenberg.NamingConventions.ValidBlockLibraryFunctionName"/>
+	<rule ref="Gutenberg.Commenting.FunctionCommentSinceTag" />


I wonder if the sniff should be renamed as now it is not limited anymore to checking the presence of the @since tag in functions? I'm not sure how this sniff is used, so I don't know if there are any implications to changing its name.

I agree. I don't think there will be any implications related to renaming the sniff. Fixed in cc2586c.

anton-vlasenko · 2024-04-11T12:59:17Z

Good job, @anton-vlasenko! I left some comments that occurred to me while checking the file changes. I focused on the changes to the sniff code. As you know, I'm not super familiar with the Gutenberg project, so some of my comments might not apply.

Thank you for your review, @rodrigoprimo!
I've updated the sniff to addreess your feedback.

rodrigoprimo · 2024-04-12T14:05:47Z

test/php/gutenberg-coding-standards/Gutenberg/Sniffs/Commenting/SinceTagSniff.php

+	 * Determines if a T_STRING token represents a function call.
+	 *
+	 * @param File $phpcs_file    The file being scanned.
+	 * @param int  $stack_pointer The position to start looking for the docblock.    *


Just a detail but there is an extra * at the end of this line

Good catch. Fixed in 9416c9f

rodrigoprimo · 2024-04-12T14:16:09Z

test/php/gutenberg-coding-standards/Gutenberg/Sniffs/Commenting/SinceTagSniff.php

+	 * @param int  $stack_pointer The position to start looking for the docblock.    *
+	 * @return bool True if the token represents a function call, false otherwise.
+	 */
+	protected static function is_function_call( File $phpcs_file, $stack_pointer ) {


I wish there was a PHPCS or PHPCSUtils method that we could use to check if the token is a function call or not, but apparently, there isn't. At least I was not able to find one when I looked for it the first time I reviewed this PR.

This could be done, but let me note that the implementation was largely copied from PHPCompatibility\Sniffs\Extensions\RemovedExtensionsSniff::process(), so it is not new code.

rodrigoprimo · 2024-04-12T14:30:27Z

test/php/gutenberg-coding-standards/Gutenberg/Sniffs/Commenting/SinceTagSniff.php

+		}
+
+		if ( isset( $tokens[ $open_bracket ]['parenthesis_closer'] ) === false ) {
+			// Not a function call.


I guess it could be a function call and the closing parenthesis is missing if the sniff is being executed during live coding. That is just a detail, and I don't think the behavior of the code should change. It should still return false.

rodrigoprimo · 2024-04-12T14:33:07Z

.../php/gutenberg-coding-standards/Gutenberg/Sniffs/Commenting/FunctionCommentSinceTagSniff.php

+	 * @param File $phpcs_file    The file being scanned.
+	 * @param int  $stack_pointer The position of the hook token in the stack.
+	 */
+	protected function process_hook( File $phpcs_file, $stack_pointer ) {


Thanks for addressing this as well. The is_function_call() method seems to be working correctly as far as I can check for the purposes needed for this sniff.

rodrigoprimo

Thanks for implementing the changes, @anton-vlasenko! It looks good to me (just reinforcing that I haven't checked the whole PR, only the sniff code).

I left a few more comments. Only one might result in a code change, but it is just a nitpick.

anton-vlasenko · 2024-04-17T13:45:09Z

Thanks for implementing the changes, @anton-vlasenko! It looks good to me (just reinforcing that I haven't checked the whole PR, only the sniff code).

I left a few more comments. Only one might result in a code change, but it is just a nitpick.

Thank you for the second review, @rodrigoprimo ! I appreciate it.
I've replied to your comments.
Would you please approve this PR if you think it's fine?

rodrigoprimo · 2024-04-18T14:18:39Z

Would you please approve this PR if you think it's fine?

I'm a little bit hesitant to approve this PR as I didn't review everything, and I'm not familiar with the Gutenberg repository. That being said, I can confirm that you addressed all the points that I raised. Thanks!

gziolo

Based on the changes applied to the PHP files, that get synced to WordPress Core, this is looking great. Awesome work bringing it to the finish line.

…ce, and enum declarations. 2. Added support for checking `@since` tags for class and trait properties; 3. Added support for checking `@since` tags for class, interface, and trait method declarations; 4. Added support for checking `@since` tags for WordPress functions that invoke hooks: `do_action()`, `do_action_ref_array()`, `apply_filters()`, `apply_filters_ref_array()`. 5. Refactored unit tests; duplicate code moved to the new `GutenbergCS\Gutenberg\Tests\AbstractSniffUnitTest` class.

anton-vlasenko force-pushed the add/since-tag-sniff-class-methods-support branch 2 times, most recently from b341fd7 to 050093b Compare April 3, 2024 13:40

anton-vlasenko marked this pull request as ready for review April 3, 2024 13:58

anton-vlasenko requested a review from ajitbohra as a code owner April 3, 2024 13:58

anton-vlasenko added [Type] Code Quality Issues or PRs that relate to code quality [Package] Block library /packages/block-library and removed [Package] Block library /packages/block-library labels Apr 3, 2024

anton-vlasenko requested review from gziolo, hellofromtonya, ironprogrammer and azaozz April 3, 2024 19:55

rodrigoprimo reviewed Apr 5, 2024

View reviewed changes

anton-vlasenko requested a review from rodrigoprimo April 11, 2024 12:59

rodrigoprimo reviewed Apr 12, 2024

View reviewed changes

anton-vlasenko requested a review from rodrigoprimo April 17, 2024 13:45

gziolo approved these changes May 10, 2024

View reviewed changes

anton-vlasenko force-pushed the add/since-tag-sniff-class-methods-support branch 3 times, most recently from 0bc13f0 to 115863a Compare May 10, 2024 14:50

anton-vlasenko added 3 commits May 10, 2024 16:52

anton-vlasenko force-pushed the add/since-tag-sniff-class-methods-support branch from 115863a to 0b9db72 Compare May 10, 2024 14:56

anton-vlasenko merged commit e3eab49 into trunk May 10, 2024
61 checks passed

anton-vlasenko deleted the add/since-tag-sniff-class-methods-support branch May 10, 2024 16:02

github-actions bot added this to the Gutenberg 18.4 milestone May 10, 2024

anton-vlasenko mentioned this pull request May 22, 2024

Enforce @since tags in DocBlocks WordPress/WordPress-Coding-Standards#2446

Closed

4 tasks

Enforce @since tags in /packages/block-serialization-default-parser/ and other files #60007

Enforce @since tags in /packages/block-serialization-default-parser/ and other files #60007

Conversation

anton-vlasenko commented Mar 19, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions bot commented Mar 27, 2024 • edited Loading

github-actions bot commented Apr 3, 2024 • edited Loading

anton-vlasenko commented Apr 3, 2024 • edited Loading

rodrigoprimo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anton-vlasenko Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anton-vlasenko Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anton-vlasenko Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anton-vlasenko commented Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anton-vlasenko Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigoprimo left a comment

Choose a reason for hiding this comment

anton-vlasenko commented Apr 17, 2024

rodrigoprimo commented Apr 18, 2024

gziolo left a comment

Choose a reason for hiding this comment

anton-vlasenko commented Mar 19, 2024 •

edited

Loading

github-actions bot commented Mar 27, 2024 •

edited

Loading

github-actions bot commented Apr 3, 2024 •

edited

Loading

anton-vlasenko commented Apr 3, 2024 •

edited

Loading

anton-vlasenko Apr 8, 2024 •

edited

Loading

anton-vlasenko Apr 17, 2024 •

edited

Loading

anton-vlasenko Apr 8, 2024 •

edited

Loading

anton-vlasenko commented Apr 11, 2024 •

edited

Loading

anton-vlasenko Apr 17, 2024 •

edited

Loading