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

Enforce @since tags in block-library/src/*/*.php files #59700

Merged
merged 80 commits into from
Mar 13, 2024

Conversation

anton-vlasenko
Copy link
Contributor

@anton-vlasenko anton-vlasenko commented Mar 8, 2024

What?

This PR introduces a new sniff called FunctionCommentSinceTagSniff to ensure that PHP functions have a valid @since tag in the docblock.

The sniff skips checking files in __experimental block-library packages.

Fixes #55777.

Why?

@since tags are helpful for tracking origins and how long things have been in Core.

How?

  • Defines a new sniff class FunctionCommentSinceTagSniff within the GutenbergCS\Gutenberg\Sniffs\Commenting namespace.
  • Adds unit tests for the FunctionCommentSinceTagSniff sniff.
  • Updates existing block-library functions so that they have valid @since tags.

Testing Instructions

  1. Try to define a PHP function somewhere in the block-library/src/*/*.php without a @since tag.
  2. Make sure that CI checks fail.

Testing Instructions for Keyboard

Screenshots or Screencast

Copy link

github-actions bot commented Mar 8, 2024

Flaky tests detected in 9bf6c1868644024f75f08d5b5a3da7ffbf614f4a.
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/8264199919
📝 Reported issues:

@anton-vlasenko anton-vlasenko changed the title Enforcec @since tag Mar 12, 2024
@anton-vlasenko anton-vlasenko self-assigned this Mar 12, 2024
@anton-vlasenko anton-vlasenko added the [Type] Code Quality Issues or PRs that relate to code quality label Mar 12, 2024
@anton-vlasenko anton-vlasenko marked this pull request as ready for review March 12, 2024 20:22
Copy link

github-actions bot commented Mar 12, 2024

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: gziolo <gziolo@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: getdave <get_dave@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: kebbet <kebbet@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
Copy link
Contributor Author

@kebbet @gziolo I would appreciate your feedback and thoughts.
Thank you.

@@ -8,6 +8,8 @@
/**
* Renders the `core/media-text` block on server.
*
* @since 6.6.0
Copy link
Contributor Author

@anton-vlasenko anton-vlasenko Mar 12, 2024

Choose a reason for hiding this comment

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

This is the only place I'm not sure about. These functions haven't been included in WordPress Core yet.

@@ -54,6 +56,8 @@ function render_block_core_media_text( $attributes, $content ) {

/**
* Registers the `core/media-text` block renderer on server.
*
* @since 6.6.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only place I'm not sure about. These functions haven't been included in WordPress Core yet.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see it in WordPress core, so that would be correct. The PHP file was added 5 days ago 👍🏻

@anton-vlasenko anton-vlasenko changed the title Enforce @since tag in block-library/src/*/*.php files Mar 12, 2024
@anton-vlasenko anton-vlasenko added [Type] Enhancement A suggestion for improvement. [Package] Block library /packages/block-library and removed [Type] Enhancement A suggestion for improvement. labels Mar 12, 2024
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Impressive detective work adding all missing @since annotations. It must have taken some time to track down the proper versions. I'm not going to validate these numbers and will put trust in you and the collective knowledge of the community that can always follow-up to correct them 😄

The code looks good. I'm starting to get more familiar with PHP sniffs and more open to approve 😄

I left a comment with a suggestion to cover more files that I know we sync with WP core, but it isn't a blocker. In practice, it would require some changes to when to run is_experimantal_package, as it only applies to the packages/block-library. So maybe, another PR.

@@ -54,6 +56,8 @@ function render_block_core_media_text( $attributes, $content ) {

/**
* Registers the `core/media-text` block renderer on server.
*
* @since 6.6.0
Copy link
Member

Choose a reason for hiding this comment

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

I don't see it in WordPress core, so that would be correct. The PHP file was added 5 days ago 👍🏻

phpcs.xml.dist Outdated
@@ -172,4 +172,8 @@
</property>
</properties>
</rule>
<rule ref="Gutenberg.Commenting.FunctionCommentSinceTag">
<!-- The sniff ensures that functions have a valid @since tag but skips checking experimental packages. -->
Copy link
Member

@gziolo gziolo Mar 13, 2024

Choose a reason for hiding this comment

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

Nice!

In another PR, we should also include /pacakages/block-serialization-default-parser/.+/*\.php$.

https://github.com/WordPress/wordpress-develop/blob/b1b97f96727f137210b942f00f85fc6449033d1d/tools/webpack/packages.js#L128-L135

There are two more special core blocks:

  • widgets/src/blocks/legacy-widget/index.php
  • widgets/src/blocks/widget-group/index.php

https://github.com/WordPress/wordpress-develop/blob/b1b97f96727f137210b942f00f85fc6449033d1d/tools/webpack/blocks.js#L38-L48

@youknowriad, what do we miss here? Should we discuss in a separate issue what else we could copy from Gutenberg to the core and put under more pressure to follow WP coding standards? 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

It is interesting that @getdave opened a discussion about this PR yesterday #59786


Yeah, so these files would be good to address next but I wonder if all the php files in Gutenberg should have these rules applied (Manual backports include a lot of copy/pasting of php files)

Copy link
Contributor Author

@anton-vlasenko anton-vlasenko Mar 13, 2024

Choose a reason for hiding this comment

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

Thank you for your feedback, @gziolo. Your review is greatly appreciated.
I'd prefer to implement checking files in /pacakages/block-serialization-default-parser and other folders in a follow-up issue as this would allow for incremental improvements while ensuring that the enforcement of @since tags in the Block Library is already in place.

Follow-up issue: #59826

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is definitely another issue/PR. We better introduce these improvements gradually so we can start catching issues for covered files.

@kebbet
Copy link
Contributor

kebbet commented Mar 13, 2024

What a detectives work! Great with progress on the @since-tag, and creating a sniff for it! Wow! 👏🏼

@youknowriad
Copy link
Contributor

Awesome work here.

Copy link
Member

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

A custom sniff for enforcing @since tags sounds really useful!

I definitely encourage you to contribute this to WPCS, as this is important for WordPress core as well and even plugins and themes. It's the right thing to do.

See WordPress/WordPress-Coding-Standards#1994

@anton-vlasenko anton-vlasenko enabled auto-merge (squash) March 13, 2024 17:53
@anton-vlasenko anton-vlasenko merged commit 86b166c into trunk Mar 13, 2024
57 checks passed
@anton-vlasenko anton-vlasenko deleted the try/enforce-since-tag-sniff branch March 13, 2024 18:01
@github-actions github-actions bot added this to the Gutenberg 18.0 milestone Mar 13, 2024
@anton-vlasenko
Copy link
Contributor Author

anton-vlasenko commented Mar 13, 2024

Great work. I can't believe you found all the @since releases for al those 🤯

I'm of the opinion we should ship and if we find any that are inaccurate we can modify them as needed.

I would encourage you to leave your opinion on the discussion I raised about the need to implement these exact types of automated linting in the project. It would be hugely valuable if you were able to continue down this route.

Thanks again

Thank you, @getdave!
I'm glad you think we should proceed with this PR.
I will look into the issue you mentioned.

A custom sniff for enforcing @since tags sounds really useful!

I definitely encourage you to contribute this to WPCS, as this is important for WordPress core as well and even plugins and themes. It's the right thing to do.

See WordPress/WordPress-Coding-Standards#1994

Thank you for your suggestion, @swissspidy.
I've already attempted to submit some of my other sniffs to WPCS.
However, the sniff I tried to submit was Gutenberg-specific, and I received feedback from the WPCS maintainers stating that it doesn't belong in the WPCS package (meaning it won't be useful for other WordPress plugins/sites).
I agree with that assessment.
This particular sniff is also Gutenberg-specific, as it checks whether the current block is experimental.
However, if I ever work on a sniff that is more universally applicable, I will consider submitting it to WPCS.
I think that's great idea.

@swissspidy
Copy link
Member

@anton-vlasenko Yes obviously the sniffs would need to be made generic. In this instance for example there is no reason to include GB-specific stuff in it. I would just make the sniff enforce @since annotations for all functions. You can then either a) create a custom sniff in GB that extends the generic one or b) just exclude experimental block files in the config file. That seems pretty straightforward.

Agan, especially core itself would be really benefit from this and expanding the scope of the contribution would demonstrate the unity of the project instead of working in isolation. Just figured I'd chime in to share a core perspective here.

If you don't have the capacity for this I totally understand of course, so maybe we can start with leaving a comment on that issue.

@getdave
Copy link
Contributor

getdave commented Mar 14, 2024

Agan, especially core itself would be really benefit from this and expanding the scope of the contribution would demonstrate the unity of the project instead of working in isolation. Just figured I'd chime in to share a core perspective here.

Yes please 👍 The more we can unify the standards then the easier it will make syncing code from Gutenberg repo to the WP Core repo.

Personally I feel like exploring these opportunities would be a great use of contributor time as - based on my experience in this cycle - I see we all spend too much time on resolving coding standards issues which should be automated.

It's my opinion that we should consider them the same codebase in many ways. Therefore the more alignment we can bring the better. The more of this that is automated the better.

Again I'd love to hear from folks on #59786.

@anton-vlasenko
Copy link
Contributor Author

Agan, especially core itself would be really benefit from this and expanding the scope of the contribution would demonstrate the unity of the project instead of working in isolation. Just figured I'd chime in to share a core perspective here.

@swissspidy I will think about submitting this sniff to WPCS when it becomes more stable. You've made me reconsider my viewpoint on the matter, and I agree that Core could benefit from it. Thank you for sharing your thoughts.

@anton-vlasenko
Copy link
Contributor Author

Agan, especially core itself would be really benefit from this and expanding the scope of the contribution would demonstrate the unity of the project instead of working in isolation. Just figured I'd chime in to share a core perspective here.

Thank you for sharing your thoughts, @getdave. I appreciate it. I will consider submitting this sniff to WPCS after #59826 is done.

carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Mar 27, 2024
* Enforce @SInCE tags in block-library/src/*/*.php files

Co-authored-by: anton-vlasenko <antonvlasenko@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: getdave <get_dave@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block library /packages/block-library [Type] Code Quality Issues or PRs that relate to code quality
6 participants