Register block style early. #4621

spacedmonkey · 2023-06-15T13:38:04Z

Trac ticket: https://core.trac.wordpress.org/ticket/58528

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

westonruter

Just a couple driveby nits

src/wp-includes/blocks/index.php

Co-authored-by: Weston Ruter <[email protected]>

spacedmonkey · 2023-06-16T09:46:54Z

CC @aristath

felixarntz

@spacedmonkey This looks great so far. Only a bit of feedback below.

src/wp-includes/blocks/index.php

mukeshpanchal27

Great work @spacedmonkey, Left some feedback.

src/wp-includes/blocks/index.php

Co-authored-by: Mukesh Panchal <[email protected]>

mukeshpanchal27

@spacedmonkey A few last feedback, but almost good to go.

src/wp-includes/blocks.php

src/wp-includes/blocks/index.php

mukeshpanchal27

@spacedmonkey Left some feedback.

src/wp-includes/blocks/index.php

mukeshpanchal27

Thanks @spacedmonkey for the update. It look good to me.

src/wp-includes/blocks.php

felixarntz

Thanks @spacedmonkey, LGTM in terms of production code!

If you can add a few tests, that would be excellent 🎉

mukeshpanchal27

Thanks @spacedmonkey, Great work. Approved with one nit-pick.

src/wp-includes/blocks/index.php

src/wp-includes/blocks.php

spacedmonkey · 2023-06-26T11:12:31Z

@felixarntz @mukeshpanchal27 As requested, I have added unit tests. The unit tests have highlighted that the incorrectly value was being return. I moved the some the logic in register_block_style_handle around. This does two things, one skips the need to call wp_should_load_separate_core_block_assets, which resulted in a 1ms of load time. Two, calls generate_block_asset_handle and gets the correct style handle name early, so it can be used with wp_style_is.

This change now is even better of a performance improvement.

felixarntz

@spacedmonkey Production code LGTM! For the tests, please use a data provider rather than a big foreach loop in the tests themselves.

tests/phpunit/tests/blocks/registerCoreBlockStyleHandles.php

westonruter · 2023-06-26T17:10:57Z

src/wp-includes/blocks/index.php

+	 * @todo Replace `WP_DEBUG` once an "in development mode" check is available in Core.
+	 */
+	if ( ! WP_DEBUG ) {
+		$transient_name = 'wp_core_block_styles';


Would this be better named:

Suggested change

$transient_name = 'wp_core_block_styles';

$transient_name = 'wp_core_block_css_files';

westonruter · 2023-06-26T17:11:16Z

src/wp-admin/includes/upgrade.php

 			update_site_meta( get_current_blog_id(), 'db_last_updated', microtime() );
 		}

+		delete_transient( 'wp_core_block_styles' );


Suggested change

delete_transient( 'wp_core_block_styles' );

delete_transient( 'wp_core_block_css_files' );

Also, per below, wouldn't site transients be more appropriate?

Suggested change

delete_transient( 'wp_core_block_styles' );

delete_site_transient( 'wp_core_block_css_files' );

@westonruter Using network options ( tranisents ) has a performance impact. I think network options are not autoloaded. So it result in a database query, slow things down. Maybe we could work this out later.

westonruter · 2023-06-26T17:17:49Z

src/wp-includes/blocks/index.php

+		$files          = get_transient( $transient_name );
+		if ( ! $files ) {
+			$files = glob( __DIR__ . '/**/**.css' );
+			set_transient( $transient_name, $files );


Seems like this could be setting a site transient instead:

Suggested change

set_transient( $transient_name, $files );

set_site_transient( $transient_name, $files );

See https://github.com/WordPress/wordpress-develop/pull/4621/files#r1242531400

felixarntz

Thanks @spacedmonkey, looks great! Good to commit IMO.

felixarntz

@spacedmonkey One small tweak here is needed now that wp_get_development_mode() is available in trunk, see below. Can you please update the code as suggested to get rid of the todo comment?

It would be nice to add a simple test to ensure that the transient is used only when the development mode is not "core", so if you can implement that before the beta release, that would be awesome, though not a blocker IMO. For reference, it could be similar to e.g. 4a16702#diff-a2793e725e03902d8a665c105916351677858253a237e4d08bdc1c01f8c3d671R38-R61.

src/wp-includes/blocks/index.php

spacedmonkey · 2023-06-26T21:17:37Z

Committed d8409a2

Register block style early.

6bae079

spacedmonkey self-assigned this Jun 15, 2023

spacedmonkey added 3 commits June 15, 2023 15:23

Move logic

935d846

remove logic.

2ce21c6

Cleaner logic.

8f263c8

westonruter reviewed Jun 15, 2023

View reviewed changes

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

spacedmonkey and others added 2 commits June 15, 2023 17:02

Apply suggestions from code review

d03f659

Co-authored-by: Weston Ruter <[email protected]>

Break into it's own action.

c324c00

spacedmonkey requested review from felixarntz and joemcgill June 16, 2023 09:32

Merge branch 'trunk' into fix/realpath-error-5

e6521a1

felixarntz requested changes Jun 16, 2023

View reviewed changes

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

spacedmonkey added 3 commits June 17, 2023 18:45

Feedback.

5dac270

Merge branch 'trunk' into fix/realpath-error-5

989df55

More feedback

a877315

spacedmonkey requested review from felixarntz and westonruter June 19, 2023 10:43

mukeshpanchal27 reviewed Jun 19, 2023

View reviewed changes

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

Apply suggestions from code review

3a8d4c4

Co-authored-by: Mukesh Panchal <[email protected]>

spacedmonkey requested a review from mukeshpanchal27 June 19, 2023 15:04

mukeshpanchal27 reviewed Jun 20, 2023

View reviewed changes

src/wp-includes/blocks.php Outdated Show resolved Hide resolved

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

spacedmonkey added 3 commits June 20, 2023 12:23

Merge branch 'trunk' into fix/realpath-error-5

bb712c3

Feedback.

c01d722

Reusable.

6567ae6

spacedmonkey requested a review from mukeshpanchal27 June 21, 2023 08:37

mukeshpanchal27 reviewed Jun 21, 2023

View reviewed changes

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

spacedmonkey commented Jun 21, 2023

View reviewed changes

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

Apply suggestions from code review

c76122a

mukeshpanchal27 approved these changes Jun 21, 2023

View reviewed changes

spacedmonkey mentioned this pull request Jun 23, 2023

Fix css styles in block.jsons. WordPress/gutenberg#51866

Merged

Revert committed file.

d3109b8

spacedmonkey commented Jun 23, 2023

View reviewed changes

src/wp-includes/blocks.php Outdated Show resolved Hide resolved

felixarntz approved these changes Jun 23, 2023

View reviewed changes

mukeshpanchal27 approved these changes Jun 26, 2023

View reviewed changes

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

spacedmonkey added 3 commits June 26, 2023 10:08

Merge branch 'trunk' into fix/realpath-error-5

838ccf4

Improve performance and add unit tests.

4ae7d10

Merge branch 'trunk' into fix/realpath-error-5

b4842ef

spacedmonkey force-pushed the fix/realpath-error-5 branch from cdbb241 to b4842ef Compare June 26, 2023 10:46

spacedmonkey commented Jun 26, 2023

View reviewed changes

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

spacedmonkey commented Jun 26, 2023

View reviewed changes

src/wp-includes/blocks.php Outdated Show resolved Hide resolved

Apply suggestions from code review

f2d9152

spacedmonkey added 3 commits June 26, 2023 17:06

Merge branch 'trunk' into fix/realpath-error-5

d584473

Fix lints.

27d9cf9

Fix comment

2868761

felixarntz reviewed Jun 26, 2023

View reviewed changes

tests/phpunit/tests/blocks/registerCoreBlockStyleHandles.php Outdated Show resolved Hide resolved

tests/phpunit/tests/blocks/registerCoreBlockStyleHandles.php Outdated Show resolved Hide resolved

tests/phpunit/tests/blocks/registerCoreBlockStyleHandles.php Outdated Show resolved Hide resolved

Convert code to use a dataProvider.

9f95e93

westonruter reviewed Jun 26, 2023

View reviewed changes

spacedmonkey added 2 commits June 26, 2023 18:37

Workaround.

a15082f

Change transient name.

6f1bf01

spacedmonkey mentioned this pull request Jun 26, 2023

Add theme styles. spacedmonkey/wordpress-develop#4

Closed

felixarntz approved these changes Jun 26, 2023

View reviewed changes

felixarntz reviewed Jun 26, 2023

View reviewed changes

src/wp-includes/blocks/index.php Outdated Show resolved Hide resolved

spacedmonkey added 2 commits June 26, 2023 21:35

Use wp_get_development_mode.

5e0fed6

Merge branch 'trunk' into fix/realpath-error-5

519ba79

spacedmonkey closed this Jun 26, 2023

	$transient_name = 'wp_core_block_styles';
	$transient_name = 'wp_core_block_css_files';

	delete_transient( 'wp_core_block_styles' );
	delete_transient( 'wp_core_block_css_files' );

	delete_transient( 'wp_core_block_styles' );
	delete_site_transient( 'wp_core_block_css_files' );

	set_transient( $transient_name, $files );
	set_site_transient( $transient_name, $files );

Register block style early. #4621

Register block style early. #4621

Uh oh!

Conversation

spacedmonkey commented Jun 15, 2023

Trac ticket: https://core.trac.wordpress.org/ticket/58528

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

spacedmonkey commented Jun 16, 2023

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spacedmonkey commented Jun 26, 2023

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

westonruter Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

westonruter Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

westonruter Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

spacedmonkey Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

westonruter Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

spacedmonkey Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment