-
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?
Changes from all commits
26c603c
9b8e410
52fa2d6
a927a03
6fee48e
bee63c5
e12aece
8c99034
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,9 +13,15 @@ | |
| define( 'BLOCKS_PATH', ABSPATH . WPINC . '/blocks/' ); | ||
|
|
||
| // Include files required for core blocks registration. | ||
| require BLOCKS_PATH . 'legacy-widget.php'; | ||
| require BLOCKS_PATH . 'widget-group.php'; | ||
| require BLOCKS_PATH . 'require-dynamic-blocks.php'; | ||
| if ( file_exists( BLOCKS_PATH . 'legacy-widget.php' ) ) { | ||
youknowriad marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| require BLOCKS_PATH . 'legacy-widget.php'; | ||
| } | ||
| if ( file_exists( BLOCKS_PATH . 'widget-group.php' ) ) { | ||
| require BLOCKS_PATH . 'widget-group.php'; | ||
| } | ||
| if ( file_exists( BLOCKS_PATH . 'require-dynamic-blocks.php' ) ) { | ||
| require BLOCKS_PATH . 'require-dynamic-blocks.php'; | ||
| } | ||
|
|
||
| /** | ||
| * Registers core block style handles. | ||
|
|
@@ -43,6 +49,9 @@ function register_core_block_style_handles() { | |
|
|
||
| static $core_blocks_meta; | ||
| if ( ! $core_blocks_meta ) { | ||
| if ( ! file_exists( BLOCKS_PATH . 'blocks-json.php' ) ) { | ||
| return; | ||
| } | ||
| $core_blocks_meta = require BLOCKS_PATH . 'blocks-json.php'; | ||
| } | ||
|
|
||
|
|
@@ -150,6 +159,9 @@ static function ( $file ) use ( $normalized_blocks_path ) { | |
| * @since 5.5.0 | ||
| */ | ||
| function register_core_block_types_from_metadata() { | ||
| if ( ! file_exists( BLOCKS_PATH . 'require-static-blocks.php' ) ) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| return; | ||
| } | ||
| $block_folders = require BLOCKS_PATH . 'require-static-blocks.php'; | ||
| foreach ( $block_folders as $block_folder ) { | ||
| register_block_type_from_metadata( | ||
|
|
@@ -169,6 +181,9 @@ function register_core_block_types_from_metadata() { | |
| * @since 6.7.0 | ||
| */ | ||
| function wp_register_core_block_metadata_collection() { | ||
| if ( ! file_exists( BLOCKS_PATH . 'blocks-json.php' ) ) { | ||
| return; | ||
| } | ||
| wp_register_block_metadata_collection( | ||
| BLOCKS_PATH, | ||
| BLOCKS_PATH . 'blocks-json.php' | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5227,6 +5227,11 @@ function wp_pre_kses_less_than_callback( $matches ) { | |
| * @return string Filtered text to run through KSES. | ||
| */ | ||
| function wp_pre_kses_block_attributes( $content, $allowed_html, $allowed_protocols ) { | ||
| // If the block parser isn't available, skip block attribute filtering. | ||
| if ( ! class_exists( 'WP_Block_Parser' ) ) { | ||
| return $content; | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. What if something in the build process fails?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is correct though, there doesn't seem any other case for me where the parser is not available outside
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we now have
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
So basically, the only way for a potential issue to exist here is the following:
I think the chances for this to happen are basically zero. |
||
|
|
||
| /* | ||
| * `filter_block_content` is expected to call `wp_kses`. Temporarily remove | ||
| * the filter to avoid recursion. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,7 +48,7 @@ const COPY_CONFIG = { | |
| source: 'scripts', | ||
| destination: 'js/dist', | ||
| copyDirectories: true, // Copy subdirectories | ||
| patterns: [ '*.js', '*.js.map' ], | ||
| patterns: [ '*.js' ], | ||
| // Rename vendors/ to vendor/ when copying | ||
| directoryRenames: { | ||
| vendors: 'vendor', | ||
|
|
@@ -916,25 +916,21 @@ async function main() { | |
| // Only copy react-jsx-runtime files, skip react and react-dom | ||
| const vendorFiles = fs.readdirSync( src ); | ||
| let copiedCount = 0; | ||
| fs.mkdirSync( dest, { recursive: true } ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reviewing this last commit, should we check if vendorFiles is not empty before calling fs.mkdirSync to avoid creating the dir if there are no files? |
||
| for ( const file of vendorFiles ) { | ||
| if ( file.startsWith( 'react-jsx-runtime' ) ) { | ||
| if ( | ||
| file.startsWith( 'react-jsx-runtime' ) && | ||
| file.endsWith( '.js' ) | ||
| ) { | ||
| const srcFile = path.join( src, file ); | ||
| const destFile = path.join( dest, file ); | ||
| fs.mkdirSync( dest, { recursive: true } ); | ||
|
|
||
| if ( | ||
| file.endsWith( '.js' ) && | ||
| ! file.endsWith( '.js.map' ) | ||
| ) { | ||
| let content = fs.readFileSync( | ||
| srcFile, | ||
| 'utf8' | ||
| ); | ||
| content = removeSourceMaps( content ); | ||
| fs.writeFileSync( destFile, content ); | ||
| } else { | ||
| fs.copyFileSync( srcFile, destFile ); | ||
| } | ||
|
|
||
| let content = fs.readFileSync( | ||
| srcFile, | ||
| 'utf8' | ||
| ); | ||
| content = removeSourceMaps( content ); | ||
| fs.writeFileSync( destFile, content ); | ||
| copiedCount++; | ||
| } | ||
| } | ||
|
|
@@ -955,9 +951,7 @@ async function main() { | |
|
|
||
| for ( const file of packageFiles ) { | ||
| if ( | ||
| /^index\.(js|js\.map|min\.js|min\.js\.map|min\.asset\.php)$/.test( | ||
| file | ||
| ) | ||
| /^index\.(js|min\.js|min\.asset\.php)$/.test( file ) | ||
| ) { | ||
| const srcFile = path.join( src, file ); | ||
| // Replace 'index.' with 'package-name.' | ||
|
|
@@ -972,41 +966,31 @@ async function main() { | |
| } ); | ||
|
|
||
| // Apply source map removal for .js files | ||
| if ( | ||
| file.endsWith( '.js' ) && | ||
| ! file.endsWith( '.js.map' ) | ||
| ) { | ||
| if ( file.endsWith( '.js' ) ) { | ||
| let content = fs.readFileSync( | ||
| srcFile, | ||
| 'utf8' | ||
| ); | ||
| content = removeSourceMaps( content ); | ||
| fs.writeFileSync( destPath, content ); | ||
| } else { | ||
| // Copy other files as-is | ||
| // Copy other files as-is (.min.asset.php) | ||
| fs.copyFileSync( srcFile, destPath ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } else if ( | ||
| entry.isFile() && | ||
| /\.(js|js\.map)$/.test( entry.name ) | ||
| entry.name.endsWith( '.js' ) | ||
| ) { | ||
| // Copy root-level JS files | ||
| const dest = path.join( scriptsDest, entry.name ); | ||
| fs.mkdirSync( path.dirname( dest ), { recursive: true } ); | ||
|
|
||
| if ( | ||
| entry.name.endsWith( '.js' ) && | ||
| ! entry.name.endsWith( '.js.map' ) | ||
| ) { | ||
| let content = fs.readFileSync( src, 'utf8' ); | ||
| content = removeSourceMaps( content ); | ||
| fs.writeFileSync( dest, content ); | ||
| } else { | ||
| fs.copyFileSync( src, dest ); | ||
| } | ||
| let content = fs.readFileSync( src, 'utf8' ); | ||
| content = removeSourceMaps( content ); | ||
| fs.writeFileSync( dest, content ); | ||
| } | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.