Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 20 months ago

#54647 closed defect (bug) (fixed)

Blocks: Add support for themes to block API v2 with assets in block.json

Reported by: whoisnegrello's profile whoisnegrello Owned by: gziolo's profile gziolo
Milestone: 6.0 Priority: normal
Severity: normal Version: 5.5
Component: Editor Keywords: has-patch needs-testing has-unit-tests needs-dev-note
Focuses: Cc:

Description

Issue
When you register blocks with block.json in your theme, yo had a assets's URL error because use plugins_url() by default.

Solution
Modify register_block_script_handle()and register_block_style_handle()to validate path of assets and return this correctly.

In this way it adds support to register blocks from themes.

Attachments (1)

fix.diff (3.0 KB) - added by whoisnegrello 3 years ago.

Download all attachments as: .zip

Change History (23)

This ticket was mentioned in PR #2065 on WordPress/wordpress-develop by whoisnegrello.


3 years ago
#1

Issue
When you register blocks with block.json in your theme, yo had a assets's URL error because use plugins_url() by default.

Solution
Modify register_block_script_handle()and register_block_style_handle()to validate path of assets and return this correctly.
In this way it adds support to register blocks from themes.

Trac ticket: #54647

@whoisnegrello
3 years ago

#2 @audrasjb
3 years ago

  • Milestone changed from Awaiting Review to 5.9

Moving for 5.9 consideration, as it seems it was introduced in 5.9 (which would need confirmation).

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 years ago

#4 @audrasjb
3 years ago

  • Milestone changed from 5.9 to Awaiting Review
  • Version changed from trunk to 5.5

Thanks for your patch and welcome to WordPress Trac @whoisnegrello !

Given this patch still needs some testing and that the issue was introduced in 5.5 (see [48141]), and also since today is 5.9 Release Candidate 1, let's move it back to the Awaiting review queue.

#5 @audrasjb
3 years ago

  • Keywords needs-testing added

#6 @whoisnegrello
3 years ago

Hello, @audrasjb!
Can i help with something on this ticket?
Tell me if i can help with anything.
Greetings

#7 @audrasjb
3 years ago

  • Milestone changed from Awaiting Review to 6.0

Hi, the patch looks good at a glance. It was removed from 5.9 because it was to close from the end of the beta cycle, but now we can safely move the ticket for 6.0 consideration :)

Detailing the exact steps to reproduce the issue would help a lot, so the Test team could make sure the issue is reproductible and the patch properly fix it.

(please also make sure the patch still applies cleanly against trunk)

#8 @ocean90
3 years ago

  • Keywords close added
  • Milestone changed from 6.0 to Awaiting Review

I'd say that this was and is by design. Blocks are not supposed to be registered in themes. Instead you should have dedicated plugins which register blocks. The WordPress.org theme directory also doesn't allow blocks to be included and tools like @wordpress/create-block make it quite easy to create plugins for blocks.

fix.diff has a few issues: It hardcodes the directory names for plugins and themes and there are more functions which only expect blocks to be in plugins, for example _wp_multiple_block_styles().

#9 @whoisnegrello
3 years ago

Ok, i understand.
Usually i work blocks in plugins but i thought that it was a good idea to particular cases.
Thanks for watch it.

#10 @fabiankaegy
2 years ago

Thanks @ocean90 for flagging that I created a duplicate ticket.

I have also created a patch here: https://github.com/WordPress/wordpress-develop/pull/2494

As far as the rationale for not including blocks in themes goes I agree with it from the best practices standpoint. Yes, themes should only worry about the visual representation, and plugins should worry about the functionality.

However, there sometimes are cases where that distinction isn't that clear. Especially in purpose build custom themes it does arise, that blocks are created as layout tools that wouldn't work without the theme's styling because the block essentially is the foundational building block of the theme.

A workaround for this can be found here for example: https://github.com/10up/wp-scaffold/blob/a2586e31dcea350faa6d28373e606c17a045d44d/themes/10up-theme/includes/blocks.php#L51 where filtering the plugins_url allows a hacky way to achieve the same result.

I don't think it should be necessary to have such hacky workarounds in place. Yes, the best practice should always be to put them into plugins. But there are special use-cases where it also makes sense to allow it for custom-tailored themes.

This was also discussed in the third installment of the Core Gutenberg Developer Hours between @gziolo and myself.

#11 @gziolo
2 years ago

Just wanted to share that @fabiankaegy described very valid use cases during the Gutenberg Developer Hours. I always assumed we should optimize the experience to integrate blocks with plugins and the Block Directory. However, if that's a common practice to work around the existing API to make blocks work with themes then we should reconsider the approach taken.

#12 @fabiankaegy
2 years ago

@ocean90 I've also included the feedback you've had on the original patch and made sure that all the tests are passing.

#13 @peterwilsoncc
2 years ago

  • Keywords close removed
  • Milestone changed from Awaiting Review to 6.0

I'm putting this on the 6.0 milestone as I agree including blocks in themes is a legitimate workflow in enough circumstances that WP ought to support it. Particularly in the case of client work and/or dynamic blocks.

This ticket was mentioned in PR #2494 on WordPress/wordpress-develop by fabiankaegy.


2 years ago
#14

  • Keywords has-unit-tests added

Allows the registration of blocks using block.json to register assets via the script, editorScript, viewScript, style or editorStyle keys from within a Theme. Previously this only worked from within a plugin / mu-plugin.

Trac ticket: https://core.trac.wordpress.org/ticket/54647 & https://core.trac.wordpress.org/ticket/55513

fabiankaegy commented on PR #2494:


2 years ago
#15

As part of this PR @gziolo and myself tried to add more robust tests for the assets that get registered via block.json files in the theme.

We've tried to actually test the src url of the registered script instead of only the handle:
{{{php
$expected_script_handle = 'block-theme-example-block-view-script';
$this->assertSame( 'http://example.org/wp-content/themes/block-theme/blocks/example-block/view.js', wp_scripts()->query( $expected_script_handle )->src );
}}}

However the test setup does not seem to have stubs in place for the get_theme_file_uri function calls which resulted in the tests passing successfully when running the test suite locally but failing when the tests were run in CI.

After some considerable time debugging we have decided to forgo the addition of these additional tests for now and they have been removed from the PR.

However it would be nice to further stub out the various theme functions so that testing themes gets a little easier and that we can more confidently test theme functionality.

#16 @gziolo
2 years ago

  • Keywords needs-dev-note added

#17 @gziolo
2 years ago

  • Owner set to gziolo
  • Resolution set to fixed
  • Status changed from new to closed

In 53091:

Editor: Allow registration of blocks that include assets from within a theme

Fixes the issue when you register blocks with block.json in your theme. There is no longer an assets's URL error because it resolves correctly in relation to the theme where it is located.

Props fabiankaegy, ocean90, whoisnegrello, audrasjb, peterwilsoncc,
Fixes #54647, #55513.

gziolo commented on PR #2065:


2 years ago
#19

We landed a similar patch with https://github.com/WordPress/wordpress-develop/commit/9a7a11a74f0bba15cc36243b2e24a10fc4736401. @fabiankaegy , can you confirm that all changes that were added in this PR are covered?

#20 @matveb
2 years ago

Apologies for seeing this late, but I have to say the #55513 implementation doesn't make a lot of conceptual sense to me. While arguments can be made for registering blocks from themes, it doesn't stand to reason that the block.json mechanics should expand to include them. That system optimizes for the distribution of blocks. For cases where a custom plugin cannot be developed alongside a custom theme, there's still access to register_block_type(), which seems more straightforward for using from a theme's functions file as a bundled requirement. I'm curious what the need is for using block.json registration from a theme perspective.

#21 @fabiankaegy
2 years ago

@matveb Apologies I just now also saw your comment here on this ticket.

Yes, it always has been possible to register blocks via the manual register_block_type mechanism. The reason for adding support for blocks using block.json from within themes from my perspective is, that especially when you are working with dynamic blocks, the overhead of having to keep your attribute declaration and overall block registration in sync between the PHP and JS version is a pain. This is also why the block.json mechanism got introduced in the first place. And since there already were hacky solutions like being able to filter the plugins_url out there in the world that made it work to use the more modern simplified approach, I don't see the reason for keeping this functionality from anybody that has a reasonable rationale for including a block in a custom build theme.

jack-h2ml commented on PR #2494:


20 months ago
#22

My apologies for commenting well after the fact, please do let me know if this is not an appropriate course of action.

The implementation above does not work within the Parent / Child theme paradigm. The changes introduced within wp-includes/blocks.php do not cover the use case whereby a block may be registered within a parent theme, to be used by subsequent child themes.

I have raised an issue covering this in more detail over on the Gutenberg repo WordPress/gutenberg/#47470

Note: See TracTickets for help on using tickets.