Blocks: Re-register core blocks via build copy prefixing #13521

aduth · 2019-01-26T04:53:20Z

This pull request seeks to try an approach for re-registering the core blocks as a Gutenberg-specific variant, while still allowing core to copy those distributed through the @wordpress/block-library module. The technique is rather primitive: using CopyWebpackPlugin, it copies PHP files from the block-library package, transforming them by looking for function definitions (by pattern) to prefix with gutenberg_. Gutenberg deregisters the core-registered blocks, and registers these instead. Since it's built-in to the Webpack pipeline, it also respects rebuilds on changes during npm run dev.

Testing instructions:

Run npm run dev
Make a change to a block-library PHP file
Observe that changes take effect

aduth · 2019-02-08T20:59:57Z

In 68a592b, I considered a discovery-based implementation, rather than explicitly listing each of the blocks (doing so after personally neglecting to include the second of two entries for the new "Search" block in a rebase). I'm not totally convinced about it yet, partly because of the magical implications and the possible "trust" implications of discovery-based file loading (though it's something we've done previously in #2014, and later removed in #10147).

youknowriad · 2019-02-11T08:25:32Z

lib/load.php

+			$registry->unregister( $block_name );
+		}
+
+		require $blocks_dir . $file;


So this function is called on the init hook and the files it requires also trigger functions on the init hook. Isn't it too late for the init hooks added in those files to trigger properly?

So this function is called on the init hook and the files it requires also trigger functions on the init hook. Isn't it too late for the init hooks added in those files to trigger properly?

Mmm, I'd have sworn I'd seen somewhere we'd used a similar implementation, but in mind of the following, I think it'd be expected to not work. If we handle init here at an earlier priority, it may not be an issue though.

https://github.com/WordPress/wordpress-develop/blob/83275aa/tests/phpunit/tests/actions.php#L350

https://core.trac.wordpress.org/ticket/17817

https://core.trac.wordpress.org/ticket/38011#comment:2

I figured out that my comment is (#13521 (comment)) related to what @youknowriad raised. When I change the code to execute the block's registration immediately then all blocks load properly.

Isn't it too late for the init hooks added in those files to trigger properly?

That should work ever since WP_Hook was introduced.

That should work ever since WP_Hook was introduced.

Can you clarify what you mean @swissspidy ? The current issue is that we want to load the blocks implementations at the same priority as the core registration, which based on the aforementioned test cases does not appear to be a supported use-case for hooks.

gziolo

I see some REST API errors for Archives, RSS and Latest Comments blocks which use SeverSideRender component:

gziolo · 2019-02-11T07:38:37Z

lib/load.php

+		}
+
+		// Derive the assumed block name by file path.
+		$block_name = 'core/' . $parts['filename'];


It's not always the same, for example:
https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/classic/index.js#L13

Once JSON file is there, it should fix the issue though.

It's not always the same, for example:
https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/classic/index.js#L13

Once JSON file is there, it should fix the issue though.

I'm not sure I follow what's meant by "Once JSON file is there", but I think the fact that we can't really rely on the file paths to match the block name could serve as pretty good justification to avoid the newer "discovery"-based approach in favor of an explicit list.

I was referring to #13693 and RFC for server side block registration.

It might be better to hardcode it for now.

gziolo · 2019-02-11T08:34:55Z

lib/load.php

+		// Derive the assumed block name by file path.
+		$block_name = 'core/' . $parts['filename'];
+
+		if ( $registry->is_registered( $block_name ) ) {


I was wondering why it isn't using remove_action against add_action( 'init', 'register_block_core_latest_comments' ); and others? Wouldn't be that the most expected behavior?

I was wondering why it isn't using remove_action against add_action( 'init', 'register_block_core_latest_comments' ); and others? Wouldn't be that the most expected behavior?

Maybe, but there are a few complications:

In the current stable release of WordPress, some blocks are registered immediately, not as part of an init hook (example, addressed as part of the changes of this pull request)

As noted in your prior review note, trying to compose these names (i.e. register_) dynamically is prone to being fragile

As noted by @youknowriad 's comment, removing a filter would need to occur at an earlier priority than the default for it to work

It's fine as is. I was mostly interested to see whether if this is technically possible without too much hassle.

aduth · 2019-02-14T22:09:13Z

Rebased to resolve conflicts with the introduction of calendar block (@jorgefilipecosta).

Restored a hard-coded blocks list (a single one at least, this time) in dfc8732.

The init action priority issue is actually a bit more difficult to resolve than I'd anticipated. If Gutenberg's re-registration occurs at an earlier priority, it will be called before core's own block registration handlers. They obviously can't be unregistered at that point, and when they are called, their own registration produces a warning about registering a block by the same name.

gziolo

I will test it again and get back soon :)

gziolo · 2019-02-15T11:50:00Z

lib/load.php

+	$registry = WP_Block_Type_Registry::get_instance();
+
+	foreach ( $block_names as $file => $block_name ) {
+		if ( ! file_exists( $blocks_dir . $file ) ) {


I wouldn't mind if this would also print some error using some WordPress magic.

gziolo · 2019-02-15T11:57:08Z

lib/load.php

+		// Derive the assumed block name by file path.
+		$block_name = 'core/' . $parts['filename'];
+
+		if ( $registry->is_registered( $block_name ) ) {


It's fine as is. I was mostly interested to see whether if this is technically possible without too much hassle.

gziolo · 2019-02-15T12:26:24Z

I'm still seeing 404 errors when trying to use those blocks which use ServerSideRender component. It might be related to the way how REST API endpoints are created:

https://github.com/WordPress/wordpress-develop/blob/d763cdf82c2b1c1493674d348e02f18cc266672a/src/wp-includes/rest-api.php#L268-L269
https://github.com/WordPress/wordpress-develop/blob/2ae5bb324d1644eebdd8110b92923a7199d2e101/src/wp-includes/rest-api/endpoints/class-wp-rest-block-renderer-controller.php#L35
https://github.com/WordPress/wordpress-develop/blob/99450a446d83872b2d02b4153e70b40cfd62131b/src/wp-includes/default-filters.php#L450

gziolo · 2019-02-15T12:38:03Z

I have no idea if this e69baad is the right approach but it fixes the issue I raised.

Let's see what Travis thinks about my hack.

Dismissing

aduth · 2019-02-15T13:19:23Z

I have no idea if this e69baad is the right approach but it fixes the issue I raised.

Let's see what Travis thinks about my hack.

It will resolve the issue of the blocks not being defined, but per #13521 (comment) will be problematic in that the default core blocks will attempt to register themselves at the normal priority and fail (with a warning notice).

Travis doesn't catch it because I haven't yet finished #13452 😄

gziolo · 2019-03-05T14:55:08Z

I stumbled upon the same issue when working on #14238. As a quick workaround, I renamed all methods to be prefixed with gutenberg_ in 625c7e7. However, it doesn't seem to be a good idea in the long run. I noticed that we never expose those PHP files in build and build-module folders when publishing to npm.

gziolo · 2019-03-19T13:12:32Z

I rebased this PR with the latest changes in master. Code wise it looks good. I hope we can make it less fragile in the near future as we introduce JSON block metadata format. Webpack based copy and replace part seems to be very sensitive but it seems like the only way to proceed at the moment.

I will give it another round of testing before adding ✅.

gziolo

I think it is finally sorted out 🎉

I tested it extensively and it works properly, Travis tends to agree with me

aduth · 2019-03-19T15:55:22Z

Might need one more change, with the block PHP now being considered as part of build output:

gutenberg/bin/build-plugin-zip.sh

Line 110 in c34d6b7

build_files=$(ls build/*/*.{js,css})

Related: #14289 (comment)

Should be an easy fix. I'll plan to make it, and give the ZIP a try.

aduth · 2019-03-20T01:19:28Z

I was able to reproduce it being an issue, but I had a bit more trouble fixing it than I'd anticipated. Just expanding the build_files to cover build/**/*.{js,css,php} yielded some unexpected errors ls: build/**/*.php: No such file or directory (despite working fine when run directly in the terminal). Inlining it into the zip command similarly produced "name not matched" warning. I'll plan to revisit tomorrow.

(Part of me also wonders whether we ought to even both whitelisting specific file extensions, especially when the git clean earlier in the packaging script will ensure that the build folder is completely reset and wouldn't contain extraneous files)

gziolo · 2019-03-25T09:28:17Z

It blocks #14533 from landing. I will have a look at the issue myself to unblock my other PR.

…w location

gziolo · 2019-03-25T13:47:30Z

38537a6 should do the trick:

aduth · 2019-03-25T14:10:14Z

@gziolo That seems to work well in my testing 👍 Also confirmed that with some local revisions to a block, they were respected when packaging the plugin and uploading it to a test site.

Generally I'm wondering why build_files is necessary as a separate variable vs. having each build/*/*.{js,css} and build/block-library/blocks/*.php as part of the zip command, though perhaps zip doesn't expand/glob in the same way as ls. In any case, I'd not want it to block moving forward here.

aduth · 2019-03-25T15:21:40Z

Thinking out loud: I was double-checking whether this would have any impact on the core tooling for bringing block implementations into WordPress. As I see it, the primary purpose for this pull request to exist is the fact that we explicitly do not want to change the process, at least as far as implementations of blocks PHP within packages/block-library to register themselves upon init default priority, without prefixed functions. The changes here are Gutenberg's workaround to have the plugin override those defaults.

aduth added the [Feature] Blocks Overall functionality of blocks label Jan 26, 2019

gziolo added this to the 5.1 (Gutenberg) milestone Feb 7, 2019

aduth force-pushed the try/reregister-core-block-types branch from 5b5c05c to 9c42ec1 Compare February 8, 2019 20:38

aduth requested review from ajitbohra, gziolo, jorgefilipecosta, noisysocks, notnownikki, Soean, talldan and youknowriad as code owners February 8, 2019 20:38

aduth force-pushed the try/reregister-core-block-types branch from 9c42ec1 to 68a592b Compare February 8, 2019 20:54

youknowriad reviewed Feb 11, 2019

View reviewed changes

gziolo previously requested changes Feb 11, 2019

View reviewed changes

youknowriad mentioned this pull request Feb 14, 2019

Fix: missing className on latest comments block #13834

Merged

aduth force-pushed the try/reregister-core-block-types branch from 68a592b to dfc8732 Compare February 14, 2019 22:05

gziolo reviewed Feb 15, 2019

View reviewed changes

gziolo force-pushed the try/reregister-core-block-types branch from dfc8732 to e69baad Compare February 15, 2019 12:36

noisysocks modified the milestones: 5.1 (Gutenberg), 5.2 (Gutenberg) Feb 18, 2019

gziolo modified the milestones: 5.2 (Gutenberg), 5.3 (Gutenberg) Mar 4, 2019

gziolo mentioned this pull request Mar 5, 2019

Add the block registration RFC #13693

Merged

aduth added 5 commits March 19, 2019 13:47

Blocks: Re-register core blocks via build copy prefixing

9ba6e2b

Blocks: Discover block overrides from built artifact

7138e38

Blocks: Reimplement block reregistering as hard-coded list

3dbbbe5

Blocks: Modify priority for blocks registration during build

ea85dcf

Blocks: Move blocks reregister function to standalone file

5bad351

gziolo force-pushed the try/reregister-core-block-types branch from cb194f1 to 5bad351 Compare March 19, 2019 13:09

aduth and others added 2 commits March 19, 2019 09:40

Comma comma comma comma comma chameleon

9aa0c82

Update regex in webpack config to contain gutenberg as well

761bf1e

gziolo approved these changes Mar 19, 2019

View reviewed changes

gziolo assigned aduth Mar 22, 2019

gziolo mentioned this pull request Mar 25, 2019

Block library: Standardize align and className attributes for dynamic blocks #14533

Merged

5 tasks

Update build plugin zip script to include block PHP files from the ne…

38537a6

…w location

aduth requested review from nerrad and ntwb as code owners March 25, 2019 13:47

aduth merged commit bd381c4 into master Mar 25, 2019

aduth deleted the try/reregister-core-block-types branch March 25, 2019 15:22

youknowriad added this to the 5.4 (Gutenberg) milestone Mar 29, 2019

This was referenced Apr 1, 2019

Bump plugin version to 5.4.0-rc.1 #14735

Merged

Bump plugin version to 5.4.0 #14783

Merged

aduth mentioned this pull request Apr 24, 2019

Framework: Travis build performance #15159

Closed

14 tasks

aduth mentioned this pull request May 10, 2019

Gutenberg is not compatible with WordPress 5.0 #15565

Closed

gziolo mentioned this pull request May 21, 2019

Testing: Speed up Docker setup on Travis for e2e tests #15748

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blocks: Re-register core blocks via build copy prefixing #13521

Blocks: Re-register core blocks via build copy prefixing #13521

aduth commented Jan 26, 2019

aduth commented Feb 8, 2019

youknowriad Feb 11, 2019

aduth Feb 11, 2019

gziolo Feb 15, 2019

swissspidy Mar 5, 2019

aduth Mar 5, 2019

gziolo left a comment •

edited

Loading

gziolo Feb 11, 2019

aduth Feb 11, 2019

gziolo Feb 12, 2019

gziolo Feb 11, 2019

aduth Feb 11, 2019

gziolo Feb 15, 2019

aduth commented Feb 14, 2019

gziolo left a comment

gziolo Feb 15, 2019

gziolo Feb 15, 2019

gziolo commented Feb 15, 2019 •

edited

Loading

gziolo commented Feb 15, 2019 •

edited

Loading

aduth commented Feb 15, 2019

gziolo commented Mar 5, 2019

gziolo commented Mar 19, 2019

gziolo left a comment

aduth commented Mar 19, 2019

aduth commented Mar 20, 2019 •

edited

Loading

gziolo commented Mar 25, 2019

gziolo commented Mar 25, 2019

aduth commented Mar 25, 2019

aduth commented Mar 25, 2019

Blocks: Re-register core blocks via build copy prefixing #13521

Blocks: Re-register core blocks via build copy prefixing #13521

Conversation

aduth commented Jan 26, 2019

aduth commented Feb 8, 2019

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

Choose a reason for hiding this comment

gziolo left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Feb 14, 2019

gziolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Feb 15, 2019 • edited Loading

gziolo commented Feb 15, 2019 • edited Loading

aduth commented Feb 15, 2019

gziolo commented Mar 5, 2019

gziolo commented Mar 19, 2019

gziolo left a comment

Choose a reason for hiding this comment

aduth commented Mar 19, 2019

aduth commented Mar 20, 2019 • edited Loading

gziolo commented Mar 25, 2019

gziolo commented Mar 25, 2019

aduth commented Mar 25, 2019

aduth commented Mar 25, 2019

gziolo left a comment •

edited

Loading

gziolo commented Feb 15, 2019 •

edited

Loading

gziolo commented Feb 15, 2019 •

edited

Loading

aduth commented Mar 20, 2019 •

edited

Loading