-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
PHP lib docs: update to include information about prefixes in block PHP code #55402
Conversation
… php code (hint: don't do it)
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/README.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, thanks for expanding on the docs here!
Just left a couple of minor comments, but nothing major 🙂
lib/README.md
Outdated
Accordingly, you should avoid using plugin-specific prefixes/suffixes in any block PHP code. Core blocks in the plugin are [published as NPM packages](https://github.com/WordPress/gutenberg/blob/trunk/docs/contributors/code/release.md#packages-releases-to-npm-and-wordpress-core-updates), which Core consumes as NPM dependencies. See [block naming conventions](https://github.com/WordPress/gutenberg/tree/trunk/packages/block-library#naming-convention-for-php-functions) for more information. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth mentioning that if blocks code needs to call gutenberg versions of core functions, there's a transformation that can be used in our webpack config, to ensure that in Gutenberg the GB version is called, where in core, the core version is called?
gutenberg/tools/webpack/blocks.js
Lines 25 to 36 in 00753a7
/** | |
* We need to automatically rename some functions when they are called inside block files, | |
* but have been declared elsewhere. This way we can call Gutenberg override functions, but | |
* the block will still call the core function when updates are back ported. | |
*/ | |
const prefixFunctions = [ | |
'wp_apply_colors_support', | |
'wp_enqueue_block_support_styles', | |
'wp_get_typography_font_size_value', | |
'wp_style_engine_get_styles', | |
'wp_get_global_settings', | |
]; |
It's also mentioned in this code comment:
gutenberg/tools/webpack/blocks.js
Lines 144 to 148 in 00753a7
// Within content, search and prefix any function calls from | |
// `prefixFunctions` list. This is needed because some functions | |
// are called inside block files, but have been declared elsewhere. | |
// So with the rename we can call Gutenberg override functions, but the | |
// block will still call the core function when updates are back ported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. It would be good to surface that somewhere.
I also wanted to update block-library/README.md
to stress the points raised in this PR.
I thought block-library/README.md
might be more appropriate since it'd live and travel with that package (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought block-library/README.md might be more appropriate since it'd live and travel with that package (?)
Yep, that'd be a better place for it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should actually just bundle that with this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way — we can always include it in both places 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as-is! ✨ Happy to proofread again if you wind up wanting to add the blocks changes to this PR 🙂
Thanks for the help! I've quickly added a section about the webpack replace process in the block library README |
Perfect, looks great to me! 🙇 |
Flaky tests detected in 07b4aac. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6542161824
|
Thank you for this 👏 |
What?
Updates
lib/README.md
to include information about Gutenberg prefixes in block (hint: don't do it)Motivation: https://github.com/WordPress/gutenberg/pull/54801/files#r1361396788
Why?
To make it clear that recommendations around plugin-specific prefixes/suffixes apply to PHP files in the
/lib
directory and not block PHP code.