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 valid function names in packages/block-library/*/src/index.php files #52769

Closed
anton-vlasenko opened this issue Jul 19, 2023 · 7 comments · Fixed by #53438
Closed

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

anton-vlasenko opened this issue Jul 19, 2023 · 7 comments · Fixed by #53438
Assignees
Labels
Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts [Status] In Progress Tracking issues with work in progress [Type] Code Quality Issues or PRs that relate to code quality

Comments

@anton-vlasenko
Copy link
Contributor

anton-vlasenko commented Jul 19, 2023

What problem does this address?

Follow-up from #52696.
Functions with gutenberg_ prefixes should not be permitted in the block-library package.

Please see:

  1. Blocks: remove gutenberg refs in PHP files #51978
  2. Enforce checks against redeclaration for functions and classes #52696 (review)
  3. Implement a lint rule that ensures that functions/classes with no "gutenberg_" prefix are properly guarded #44151 (comment)

Props: @tellthemachines, @ockham

What is your proposed solution?

After the merge of #52696, a new phpcs sniff should be implemented to check that these functions do not exist in the block-library package.

@anton-vlasenko anton-vlasenko added the [Type] Code Quality Issues or PRs that relate to code quality label Jul 19, 2023
@anton-vlasenko anton-vlasenko self-assigned this Jul 19, 2023
@anton-vlasenko anton-vlasenko changed the title Check that gutenberg_ prefixed function don't exist in packages/block-library Check that gutenberg_ prefixed functions don't exist in packages/block-library Jul 25, 2023
@anton-vlasenko
Copy link
Contributor Author

I'm working on a patch for this issue.

@gziolo
Copy link
Member

gziolo commented Aug 4, 2023

Excellent initiative! Should we enforce that all function names start with consistent prefix patterns based on what we have today?

@gziolo gziolo added the Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts label Aug 4, 2023
@anton-vlasenko
Copy link
Contributor Author

Excellent initiative! Should we enforce that all function names start with consistent prefix patterns based on what we have today?

@gziolo Sorry, I'm not sure I understand. Are you referring to the (G|g)utenberg_ prefix? Or some other prefix?
And where should this enforcement take place? Throughout the entire project or in specific folders?

@gziolo
Copy link
Member

gziolo commented Aug 8, 2023

I'm talking about the same scope as this issue proposes - /packages/block-library/src/*/index.php.

Example for the Navigation block:

https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/navigation/index.php

Screenshot 2023-08-08 at 14 39 13

Functions start with:

  • block_core_navigation_
  • render_block_core_navigation_
  • register_block_core_navigation_

Aside: there is one function starting with gutenberg_, which would get detected if the linter rule described in this issue existed.

Technically speaking _navigation_ part comes from the folder name, but if that is too difficult to handle with a linting rule, it should be fine to enforce that function names can start with:

  • block_core_
  • render_block_core_
  • register_block_core_

Everything else shouldn't be allowed.

@anton-vlasenko
Copy link
Contributor Author

anton-vlasenko commented Aug 8, 2023

Thank you for providing more details in your comment, @gziolo.
Your input is much appreciated.

Technically speaking navigation part comes from the folder name, but if that is too difficult to handle with a linting rule, it should be fine to enforce that function names can start with.

Given that the linting rule I'm working on pertains specifically to Gutenberg, I don't foresee any challenges in customizing it to check if a function's name aligns with the naming rule you described, which includes the _navigation_ part (as well as other folder/file names).

I think I will need your help with testing the linting rule when it's ready.

anton-vlasenko added a commit that referenced this issue Aug 8, 2023
2. Adjust the logic per #52769 (comment).
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Aug 8, 2023
@anton-vlasenko
Copy link
Contributor Author

anton-vlasenko commented Aug 8, 2023

@gziolo I've implemented a draft version of the linting rule.
Please have a look at these code style errors: https://github.com/WordPress/gutenberg/pull/53438/files#diff-92dc6950bcaf54290625664019ea23b023cb3f156de19586612c8d3e3cf52adc
Should we just ignore these files/functions ?

@gziolo
Copy link
Member

gziolo commented Aug 9, 2023

@gziolo I've implemented a draft version of the linting rule.
Please have a look at these code style errors: https://github.com/WordPress/gutenberg/pull/53438/files#diff-92dc6950bcaf54290625664019ea23b023cb3f156de19586612c8d3e3cf52adc
Should we just ignore these files/functions ?

It's fine to ignore all these functions that don't follow patterns. We can't rename them anymore unless we keep all the old names as deprecated aliases calling new function makes that use a proper prefix. I don't think it's worth the effort if a simple code comment would allow us to achieve the same goal. The nice part is that moving forward, the developers will see hints on how to name those functions. This is great work. Thank you so much for coming up with a simple and clean solution so quickly.

anton-vlasenko added a commit that referenced this issue Aug 10, 2023
2. Adjust the logic per #52769 (comment).
@anton-vlasenko anton-vlasenko changed the title Check that gutenberg_ prefixed functions don't exist in packages/block-library Enforce valid function names in packages/block-library/*/src/index.php files Aug 10, 2023
anton-vlasenko added a commit that referenced this issue Aug 16, 2023
2. Adjust the logic per #52769 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts [Status] In Progress Tracking issues with work in progress [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants