Enforce valid function names in the packages/block-library/src//.php files #53438

anton-vlasenko · 2023-08-08T17:29:41Z

What?

This PR is focused on implementing a new PHPCS sniff, namely Gutenberg.NamingConventions.ValidBlockLibraryFunctionName.
Fixes #52769.

Why?

All the PHP function names within the ./packages/block-library/src/*/*.php files should start with one of the following prefixes:

block_core_<folder_name>
render_block_core_<folder_name>
register_block_core_<folder_name>

Here, <folder_name> represents the name of the folder where the associated file is situated. The folder name is converted to lowercase, and any characters other than letters and digits are substituted with underscores.

The introduction of this new PHPCS sniff is needed to ensure strict adherence to this naming pattern.

How?

The proposed ValidBlockLibraryFunctionNameSniff is configured to examine PHP function names contained within the ./packages/block-library/src/*/*.php files.

Whenever a function name deviates from the naming convention listed above, the PHPCS sniff will add an error to the PHPCS report.

Testing Instructions

Make sure that CI jobs pass.
Open the packages/block-library/src/calendar/index.php file in your preferred code editor.
Add a new PHP function to the file:

/**
 * A test function.
 */
function block_core_calendar_() {

}

Run the linter (npm run lint:php or composer run lint, based on your environment setup).
Notice the linter error displayed in the console:

The function name "block_core_calendar_()" is invalid because PHP function names in this file should start with one of the following prefixes: "block_core_calendar", "render_block_core_calendar", "register_block_core_calendar".

Rename the php function to block_core_clndr.
Rerun the linter.
Notice the linter error displayed in the console:

The function name "block_core_clndr()" is invalid because PHP function names in this file should start with one of the following prefixes: "block_core_calendar", "render_block_core_calendar", "register_block_core_calendar".

Rename the php function to block_core_calendar.
Rerun the linter.
Confirm the absence of linter errors in the console.
[Optional] Perform testing with other allowed prefixes, like render_block_core_calendar and register_block_core_calendar.

gziolo · 2023-08-09T08:07:49Z

See my comment in the parent issue: #52769 (comment). I think it's fine to add code comments that ignore existing violations and enforce good practices moving on.

phpcs.xml.dist

gziolo · 2023-08-10T09:19:34Z

It seems to be in a good shape, let me know when I can give a final review and approve.

anton-vlasenko · 2023-08-10T15:35:25Z

@gziolo

It seems to be in a good shape, let me know when I can give a final review and approve.

I had to address an edge case and write proper testing instructions.
Yes, please give it a final review. 🙏 Thank you!

gziolo · 2023-08-11T07:37:04Z

packages/block-library/src/avatar/index.php

@@ -87,11 +87,13 @@ function render_block_core_avatar( $attributes, $content, $block ) {
 /**
 * Generates class names and styles to apply the border support styles for
 * the Avatar block.
+ * //phpcs:disable Gutenberg.NamingConventions.ValidBlockLibraryFunctionName


This and other files are going to be copied to WordPress core. Should this rule be present in both places and get prefixed with WordPress.? Maybe it's fine to have it only in Gutenberg, just flagging the fact that these files are tricky to maintain.

@gziolo To use the WordPress. in the rule's name, it would need to become a part of the WordPress-Coding-Standards package (wpcs). However, getting this rule into the wpcs package might be challenging.

wpcs is currently undergoing a major overhaul. It's possible that you may need to wait for an undefined period of time until it's released.

Since this rule is currently essential for Gutenberg, my suggestion is to commit it to Gutenberg first. If the wpcs contributors agree that it should be included in wpcs, it can always be removed from the Gutenberg package.

To address the concern about maintainability, I will relocate the //phpcs:disable and //phpcs:enable statements from the doc blocks to prevent any interference.

We might have linting disabled on files copied from Gutenberg through WP packages, so you can double check that first before applying any changes. I guess we can live with the Gutenberg. specific rule if it doesn't throw errors on CI in WP core.

@gziolo
The //phpcs:disable Gutenberg.NamingConventions.ValidBlockLibraryFunctionName statements shouldn't generate any linter errors in Core. However, personally, I'm not fond of the idea of having these comments in Core code.
I think I will add the possibility to ignore specific functions via the linter rule's configuration in phpcs.xml.dist. This way, we can avoid cluttering Core code with these phpcs: statements. I'll make the necessary updates to the PR and let you know.

Yes, we could list these function names as exceptions in the config file. Not ideal, but it should work.

I've updated the PR.
So it can be reviewed now.
Thanks!

gziolo · 2023-08-11T07:43:28Z

packages/block-library/src/navigation/index.php

@@ -144,12 +144,14 @@ function block_core_navigation_add_directives_to_submenu( $w, $block_attributes

 	/**
 	 * Replaces view script for the Navigation block with version using Interactivity API.
+	 * //phpcs:disable Gutenberg.NamingConventions.ValidBlockLibraryFunctionName
 	 *
 	 * @param array $metadata Block metadata as read in via block.json.
 	 *
 	 * @return array Filtered block type metadata.
 	 */
 	function gutenberg_block_core_navigation_update_interactive_view_script( $metadata ) {


I'm a little bit confused here with the gutenberg_ prefix and the usage of gutenberg_should_block_use_interactivity_api. The latter will trigger a fatal error in WordPress core when the files gets copied. This code should leave outside of this file, so in that regard the lining rule confirms that it's very useful.

Aside, we should also enforce that you can't use functions prefixed with gutenberg_ with core blocks in the same set of files as they don't exist in WP core.

As for this PR, it's fine to proceed with the ignore comment, but the usage of gutenberg_should_block_use_interactivity_api needs to be replaced with code present in WP core before we start backporting changes to WP core. @luisherranz, are you aware of this issue?

Aside, we should also enforce that you can't use functions prefixed with gutenberg_ with core blocks in the same set of files as they don't exist in WP core.

Sure. This can enforced. I will update the PR, @gziolo.

@luisherranz, are you aware of this issue?

Not at all. I don't have any experience backporting stuff to WP core yet, nor with how backporting requirements affect the code of Gutenberg. But happy to fix things if you give me instructions on what needs to be done, tho.

cc: @westonruter (pinging because Weston introduced the gutenberg_should_block_use_interactivity_api function, just in case)

Related to that, I think it could be valuable to include a mandatory banner in every PHP file we know gets backported to WordPress core with the link to the document explaining all the expected requirements enforced by the complex process. It will also give a hint to folks whenever they try to update those files directly in the WordPress core which is going to be replaced the next time someone runs npm run build ...

Obviously not in this PR.

Sure. This can enforced. I will update the PR, @gziolo.

I didn't realize that the linting rule doesn't allow any other functions to be defined outside of the ones specified in the prefixes config property.
Therefore, there's no need to modify the PR to restrict the gutenberg_ prefixed functions. 🤦‍♂️

The linting rule will only permit functions that start with the block_core_, render_block_core_, and register_block_core_ prefixes (+ the directory name).

The two functions with the gutenberg_ prefix (gutenberg_block_core_file_update_interactive_view_script and gutenberg_block_core_navigation_update_interactive_view_script) were allowed because of the phpcs::disable statements before them.

Given the discussion above, I'm uncertain about the course of action for these two gutenberg_ prefixed functions. Should they be allowed for now?

cc @gziolo @luisherranz

gziolo · 2023-08-11T07:49:51Z

phpcs.xml.dist

+			<property name="prefixes" type="array">
+				<element value="block_core_"/>
+				<element value="render_block_core_"/>
+				<element value="register_block_core_"/>


I see functions that start build_template_part_, so maybe we could open also build_block_core_template_part in case folks want to use that. I don't have strong opinions on that, as they could also do block_core_tempalte_part_build.

I don't have a preference either. However, please note that the linting rule not only checks the prefix name but also the directory name, as requested here.

It's fine as is for now, but we should keep monitoring and extending the list if that is useful.

gziolo · 2023-08-11T07:58:57Z

packages/block-library/src/footnotes/index.php

@@ -85,12 +85,14 @@ function register_block_core_footnotes() {

 /**
 * Saves the footnotes meta value to the revision.
+ * //phpcs:disable Gutenberg.NamingConventions.ValidBlockLibraryFunctionName


Unrelated to this PR. I'm slightly surprised that the Footnotes block works correctly with WordPress 6.3. We don't guard calling these WP hooks when they are present in the WP core, so essential we register them twice with the Gutenberg plugin. We should double-check it and if necessary move all the code that handles WP hooks to another file in lib/compat/ with some sort of check if it was already applied and override them only if necessary. At the same time, we would have to move the same code to a different place in WordPress core. @ellatrix, what do you think?

github-actions · 2023-08-11T14:35:53Z

Flaky tests detected in 62d4d53.
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/5833436112
📝 Reported issues:

[Flaky Test] shouldn't conflict with server-side autosave #35176 in specs/editor/various/autosave.test.js

gziolo

I like how the new rule works after a few refinements. In a follow-up we should include guidelines for contributing in @wordpress/block-library that explain how functions should be named. There is already a section for that here. We could use that later to reference in the message that the linting rule prints when an error gets detected.

I'm probably not the best person to review the code implementing the rule, so feel free to wait for another review from someone better suited for that task.

anton-vlasenko · 2023-08-16T12:34:07Z

i'm probably not the best person to review the code implementing the rule, so feel free to wait for another review from someone better suited for that task.

Thank you for the code review, @gziolo.
I understand your concerns. I will ask around for help with code reviews.
Otherwise, I will rebase this PR onto trunk and merge it today evening (UTC+2).
Honestly, I don't see how anything bad could happen if this gets merged.
I've created 2 follow-up issues:
#53732
#53731

gziolo · 2023-08-16T12:41:28Z

I understand your concerns. I will ask around for help with code reviews.
Otherwise, I will merge this PR today evening (UTC+2).
Honestly, I don't see how anything bad could happen if this gets merged.

Yes, it should be all good, but I wanted to set proper expectations regarding my review. The plan sounds perfect 😄

2. Adjust the logic per #52769 (comment).

…ith an underscore.

anton-vlasenko added the [Type] Code Quality Issues or PRs that relate to code quality label Aug 8, 2023

gziolo reviewed Aug 9, 2023

View reviewed changes

phpcs.xml.dist Outdated Show resolved Hide resolved

anton-vlasenko force-pushed the try/implement-sniff-for-block-library branch from 72d0e3f to b7583d3 Compare August 10, 2023 15:09

anton-vlasenko changed the title ~~Enforce valid function names in packages/block-library/*/src/index.php files~~ Enforce valid function names in packages/block-library/*/src/index.php files Aug 10, 2023

anton-vlasenko changed the title ~~Enforce valid function names in packages/block-library/*/src/index.php files~~ Enforce valid function names in packages/block-library/*/src/index.php files Aug 10, 2023

anton-vlasenko marked this pull request as ready for review August 10, 2023 15:33

anton-vlasenko requested review from michalczaplinski, tellthemachines and ajitbohra as code owners August 10, 2023 15:33

anton-vlasenko requested review from ironprogrammer, azaozz, hellofromtonya, ockham and gziolo August 10, 2023 15:41

gziolo reviewed Aug 11, 2023

View reviewed changes

anton-vlasenko requested a review from gziolo August 11, 2023 16:04

gziolo approved these changes Aug 16, 2023

View reviewed changes

anton-vlasenko mentioned this pull request Aug 16, 2023

Naming convention for PHP functions in the block-library package should be documented #53732

Closed

anton-vlasenko added 6 commits August 16, 2023 16:05

Commit WIP.

42dd2b5

Commit WIP.

d7f7212

1. Rename sniff.

881a594

2. Adjust the logic per #52769 (comment).

Remove redundant file.

695e2bf

Fix sniff's name.

3b650ad

List allowed function names in the error message.

ce561b1

anton-vlasenko added 23 commits August 16, 2023 16:05

Add exceptions to the ValidBlockLibraryFunctionName rule.

4c12e14

Add exceptions to the ValidBlockLibraryFunctionName rule.

aed8f1b

Better sanitization of parameters.

2991293

Update docblock.

5266d88

Update changlog.

deccd15

Implement a check for edge cases where the function name terminates w…

6c3135e

…ith an underscore.

Use blockquote instead of double quote in the error message.

148c47a

Make the sanitize_directory_name method static.

aa7820f

Update the commment.

8c47356

Revert adding the phpcs:ignore statements.

93e42d7

Add ability to whilelist certain functions.

c28feeb

Revert adding the phpcs:ignore statements.

fdb738b

Revert adding the phpcs:ignore statements.

b6178b4

Add the list of allowed functions.

9076c0d

Fix typo.

b3c9010

Add comment to php.xml.dist with the reasoning for the list.

888d82f

Revert the changes as we don't use phpcs:disable statements now.

62e9c44

Fix comment.

a4bd446

Remove redundant code.

a869ff4

Improve text of the comment.

523e1ed

Improve text of the comment.

d1f38ac

Replace "backported" with "consumes" (props @hellofromtonya).

8ab646f

Remove the last sentence.

1c25491

anton-vlasenko force-pushed the try/implement-sniff-for-block-library branch from f5be78b to 1c25491 Compare August 16, 2023 14:20

anton-vlasenko merged commit c991bfc into trunk Aug 16, 2023
50 checks passed

anton-vlasenko deleted the try/implement-sniff-for-block-library branch August 16, 2023 17:15

github-actions bot added this to the Gutenberg 16.6 milestone Aug 16, 2023

anton-vlasenko changed the title ~~Enforce valid function names in packages/block-library/*/src/index.php files~~ Enforce valid function names in the packages/block-library/src/*/index.php files Aug 16, 2023

anton-vlasenko changed the title ~~Enforce valid function names in the packages/block-library/src/*/index.php files~~ Enforce valid function names in the packages/block-library/src/*/*.php files Aug 16, 2023

anton-vlasenko mentioned this pull request Aug 21, 2023

[PHP Tests] Increase parity with WordPress Core #43333

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce valid function names in the packages/block-library/src//.php files #53438

Enforce valid function names in the packages/block-library/src//.php files #53438

anton-vlasenko commented Aug 8, 2023 •

edited

Loading

gziolo commented Aug 9, 2023

gziolo commented Aug 10, 2023

anton-vlasenko commented Aug 10, 2023 •

edited

Loading

gziolo Aug 11, 2023

anton-vlasenko Aug 11, 2023

gziolo Aug 11, 2023

anton-vlasenko Aug 11, 2023 •

edited

Loading

gziolo Aug 11, 2023 •

edited

Loading

anton-vlasenko Aug 11, 2023

gziolo Aug 11, 2023

anton-vlasenko Aug 11, 2023

luisherranz Aug 11, 2023 •

edited

Loading

gziolo Aug 11, 2023 •

edited

Loading

anton-vlasenko Aug 11, 2023 •

edited

Loading

gziolo Aug 11, 2023

anton-vlasenko Aug 11, 2023 •

edited

Loading

gziolo Aug 11, 2023

gziolo Aug 11, 2023

github-actions bot commented Aug 11, 2023

gziolo left a comment •

edited

Loading

anton-vlasenko commented Aug 16, 2023 •

edited

Loading

gziolo commented Aug 16, 2023

Enforce valid function names in the packages/block-library/src/*/*.php files #53438

Enforce valid function names in the packages/block-library/src/*/*.php files #53438

Conversation

anton-vlasenko commented Aug 8, 2023 • edited Loading

What?

Why?

How?

Testing Instructions

gziolo commented Aug 9, 2023

gziolo commented Aug 10, 2023

anton-vlasenko commented Aug 10, 2023 • 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 Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

gziolo Aug 11, 2023 • 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

luisherranz Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

gziolo Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

anton-vlasenko Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anton-vlasenko Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 11, 2023

gziolo left a comment • edited Loading

Choose a reason for hiding this comment

anton-vlasenko commented Aug 16, 2023 • edited Loading

gziolo commented Aug 16, 2023

Enforce valid function names in the packages/block-library/src//.php files #53438

Enforce valid function names in the packages/block-library/src//.php files #53438

anton-vlasenko commented Aug 8, 2023 •

edited

Loading

anton-vlasenko commented Aug 10, 2023 •

edited

Loading

anton-vlasenko Aug 11, 2023 •

edited

Loading

gziolo Aug 11, 2023 •

edited

Loading

luisherranz Aug 11, 2023 •

edited

Loading

gziolo Aug 11, 2023 •

edited

Loading

anton-vlasenko Aug 11, 2023 •

edited

Loading

anton-vlasenko Aug 11, 2023 •

edited

Loading

gziolo left a comment •

edited

Loading

anton-vlasenko commented Aug 16, 2023 •

edited

Loading