-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Build: Improve Gutenberg integration workflow #10718
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
base: trunk
Are you sure you want to change the base?
Build: Improve Gutenberg integration workflow #10718
Conversation
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
abd87f6 to
2703d40
Compare
| * @since 5.5.0 | ||
| */ | ||
| function register_core_block_types_from_metadata() { | ||
| if ( ! file_exists( BLOCKS_PATH . 'require-static-blocks.php' ) ) { |
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.
What is this file? I'm not seeing it in the Gutenberg repo? Is it something dynamically built?
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.
Yes, it's built to avoid loading the block.json file from JSON, I think for performance reasons.
2703d40 to
ecb0f18
Compare
|
@TobiasBg has kindly accepted to help me test this PR properly and the unit tests in TablePress are now running without any changes. |
ecb0f18 to
e8a0aa5
Compare
| // If the block parser isn't available, skip block attribute filtering. | ||
| if ( ! class_exists( 'WP_Block_Parser' ) ) { | ||
| return $content; | ||
| } |
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.
!!!!!
do we really want to disable security protections by default? I’m thinking about cases where something changes and we forget to come back to this spot, because nobody knows it’s there, and suddenly no security mitigations are applied because the happy-path code continues looking right
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.
If there's no block parsing and no blocks, it means there's no dynamic behavior that creates security issues from blocks, which means this change is the right thing to do in this case.
Worth also nothing that the degraded mode of WordPress loading already exists a long time ago, you can't load WordPress unless you run the build command and this is not something introduced with my changes at all.
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.
If there's no block parsing and no blocks, it means there's no dynamic behavior that creates security issues from blocks, which means this change is the right thing to do in this case.
This assumes that the parser class will be missing if and only if we are in a "no block parsing and no blocks" situation, which I'm not sure is right. What if something in the build process fails?
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.
This assumes that the parser class will be missing if and only if we are in a "no block parsing and no blocks" situation, which I'm not sure is right.
This is correct though, there doesn't seem any other case for me where the parser is not available outside npm install didn't run. Also regardless of whether there are blocks or not, if the parser is not available, there's no way dynamic blocks run.
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.
we now have WP_Block_Processor which can produce parsed blocks and we have blocks serialized in JSON in post objects in the database/PHP files, don’t we? and doesn’t render_block() run fine without the parser? and if someone adds a filter to the parser class, won’t do_blocks() run fine without this missing class?
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.
Let's take a step back and contextualize what we're talking about here:
- No WordPress install in the wild will be without parser class and blocks, so no issue in this case (99.99% of the cases).
- WordPress doesn't work unbuilt (there's a message that is shown when you try to navigate to it)
- Unbuilt Wordpress-develop users will also always have the parser class and blocks, unless
npm installdidn't run, no issues here. - If
npm installdon't run at all, there will be no parser class and no blocks, which means no issues here either. - If for some reason
npm installbreaks in the middle, the copy of the "parser" happens before the copy of the "blocks" in the script, which means the chance of blocks existing and parser not existing are inexistent here.
So basically, the only way for a potential issue to exist here is the following:
- Someone is loading WordPress by loading php files directly in a script.
- That person somehow managed to have blocks in, either by including blocks manually or else
- That person somehow managed to add its own parser class
- I guess that means the person recreated Gutenberg basically.
I think the chances for this to happen are basically zero.
|
I've tested various iterations of this PR throughout the day and thanks to @youknowriad's adjustments was able to get my plugin's unit tests running again. So, for the common scenario of running unit tests on wordpress-develop, the additional |
Just want to note that the built time is actually faster than before. |
Oh, this was meant as "increased in comparison to not having to run a build step before" (for plugin unit tests, and similar). :-) That the build time for when a build is needed/desired is faster is a good side effect, of course! |
b5dd400 to
1df9f51
Compare
|
With the update of the build tool, we also now have prefixed functions, removed the lingering package.json files. |
|
@WordPress/gutenberg-core I'd appreciate a review of this PR. This unblocks folks doing unit tests without building and address all the remaining feedback on the build change. Would be good to land this one. |
daddd6b to
d1ca44e
Compare
6742d34 to
b4ed4f4
Compare
This commit improves the Gutenberg build integration in several ways: 1. Adds a new `gutenberg:sync` script that follows the same pattern as `install-changed` - storing a hash of the built ref in `.gutenberg-hash` and only rebuilding when the ref changes. This ensures that when the `gutenberg.ref` in package.json changes (e.g., after `git pull`), the next build will automatically rebuild Gutenberg. 2. Updates `postinstall` to run the full checkout + build + copy flow, so `npm install` produces a working development environment. 3. Adds conditional `file_exists()` checks around the PHP files from `wp-includes/build/` so WordPress doesn't fatal error if these files don't exist yet. 4. Restores Gutenberg's `package.json` after the build completes using `git checkout`, keeping the Gutenberg checkout clean. 5. Removes the `gutenberg:integrate` script and `gutenberg-integrate` grunt task since their functionality is now handled by `gutenberg:sync`. This simplifies the developer workflow and restores a similar flow to how package dependencies worked before - where `npm install` handles everything and subsequent builds are fast because they skip the already- built Gutenberg. See #64393. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-authored-by: Miguel Fonseca <150562+mcsf@users.noreply.github.com>
The source map files contain path references to the original Gutenberg source files which don't exist in WordPress Core. Since the .js files already have their sourceMappingURL comments stripped, these .map files serve no purpose. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The edit-post.js file comes from Gutenberg and may not always be present. jQuery is a more fundamental dependency that's always copied during the build process via Grunt's copy:vendor task. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Files ending with .js.map don't match .endsWith('.js'), so the explicit
exclusion check is unnecessary.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
More semantically correct since we're checking for a directory. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
b4ed4f4 to
e12aece
Compare
jorgefilipecosta
left a comment
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.
Tried the testing steps, tried to test this PR on playground, tried with and without Gutenberg plugin, in all the cases things worked. Did not found any issue with the code, and I agree and support the direction here, and I think it will greatly simplify gutenberg and core syncs in the future. Also opens the door to more automations on that are, e.g: I can see one for WordPress core abilities in the future.
So I think this is ready for merging getting a wider testing way before the 7.0 beta.
Create the destination directory once before copying files rather than on each iteration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Trac ticket
https://core.trac.wordpress.org/ticket/64393
Summary
This PR improves the Gutenberg build integration to address several issues reported in the ticket. The changes simplify the developer workflow and restore a flow similar to how package dependencies worked before the Gutenberg checkout-and-build approach was introduced.
Key Improvements
Automatic rebuild on ref change: Adds a new
gutenberg:syncscript that follows the same pattern asinstall-changed(which WordPress already uses for npm dependencies). It stores a hash of the built ref in.gutenberg-hashand only rebuilds when the ref changes. This means whengutenberg.refin package.json changes (e.g., aftergit pull), the nextnpm run buildwill automatically detect this and rebuild Gutenberg.Full integration on
npm install: Updatespostinstallto run the complete checkout + build + copy flow. Runningnpm installnow produces a fully working development environment with Gutenberg assets insrc/.Clean Gutenberg checkout: Restores Gutenberg's
package.jsonafter the build completes usinggit checkout, keeping the Gutenberg directory clean and avoiding uncommitted changes.Simplified scripts: Removes the
gutenberg:integratenpm script andgutenberg-integrategrunt task since their functionality is now handled bygutenberg:sync.New Flow
This is very similar to how things worked before with package dependencies -
npm installhandles everything, and subsequent builds are fast because they skip the already-built Gutenberg unless the ref has changed.Test instructions
npm installsrc/npm run build- should skip Gutenberg rebuild ("already synced")gutenberg.refin package.json to a different commitnpm run build- should detect the change and rebuild Gutenbergsrc/wp-includes/build/routes.phpand verify WordPress doesn't fatal errorFiles changed
.gitignore- Added.gutenberg-hashGruntfile.js- Addedgutenberg-synctask, removedgutenberg-integratetask, updated build taskspackage.json- Updatedpostinstall, addedgutenberg:sync, removedgutenberg:integratesrc/wp-admin/font-library.php- Updated error message to referencenpm installsrc/wp-settings.php- Added conditionalfile_exists()checks for build PHP filestools/gutenberg/build-gutenberg.js- AddedrestorePackageJson()after buildtools/gutenberg/sync-gutenberg.js- New script for hash-based rebuild detection🤖 Generated with Claude Code