Register block style early. #4621
Conversation
Co-authored-by: Weston Ruter <westonruter@google.com>
|
CC @aristath |
There was a problem hiding this comment.
@spacedmonkey This looks great so far. Only a bit of feedback below.
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
There was a problem hiding this comment.
Thanks @spacedmonkey, LGTM in terms of production code!
If you can add a few tests, that would be excellent 🎉
There was a problem hiding this comment.
Thanks @spacedmonkey, Great work. Approved with one nit-pick.
spacedmonkey
force-pushed
the
fix/realpath-error-5
branch
from
cdbb241
to
b4842ef
Compare
|
@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 This change now is even better of a performance improvement. |
There was a problem hiding this comment.
@spacedmonkey Production code LGTM! For the tests, please use a data provider rather than a big foreach loop in the tests themselves.
src/wp-includes/blocks/index.php
Outdated
| * @todo Replace `WP_DEBUG` once an "in development mode" check is available in Core. | ||
| */ | ||
| if ( ! WP_DEBUG ) { | ||
| $transient_name = 'wp_core_block_styles'; |
There was a problem hiding this comment.
Would this be better named:
| $transient_name = 'wp_core_block_styles'; | |
| $transient_name = 'wp_core_block_css_files'; |
src/wp-admin/includes/upgrade.php
Outdated
| @@ -654,6 +654,8 @@ function wp_upgrade() { | |||
| update_site_meta( get_current_blog_id(), 'db_last_updated', microtime() ); | |||
| } | |||
|
|
|||
| delete_transient( 'wp_core_block_styles' ); | |||
There was a problem hiding this comment.
| delete_transient( 'wp_core_block_styles' ); | |
| delete_transient( 'wp_core_block_css_files' ); |
There was a problem hiding this comment.
Also, per below, wouldn't site transients be more appropriate?
| delete_transient( 'wp_core_block_styles' ); | |
| delete_site_transient( 'wp_core_block_css_files' ); |
There was a problem hiding this comment.
@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.
| $files = get_transient( $transient_name ); | ||
| if ( ! $files ) { | ||
| $files = glob( __DIR__ . '/**/**.css' ); | ||
| set_transient( $transient_name, $files ); |
There was a problem hiding this comment.
Seems like this could be setting a site transient instead:
| set_transient( $transient_name, $files ); | |
| set_site_transient( $transient_name, $files ); |
There was a problem hiding this comment.
There was a problem hiding this comment.
@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.
|
Committed d8409a2 |
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.