Register block style early.#4621
Conversation
westonruter
left a comment
There was a problem hiding this comment.
Just a couple driveby nits
Co-authored-by: Weston Ruter <westonruter@google.com>
|
CC @aristath |
felixarntz
left a comment
There was a problem hiding this comment.
@spacedmonkey This looks great so far. Only a bit of feedback below.
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Great work @spacedmonkey, Left some feedback.
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
mukeshpanchal27
left a comment
There was a problem hiding this comment.
@spacedmonkey A few last feedback, but almost good to go.
mukeshpanchal27
left a comment
There was a problem hiding this comment.
@spacedmonkey Left some feedback.
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Thanks @spacedmonkey for the update. It look good to me.
felixarntz
left a comment
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 🎉
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Thanks @spacedmonkey, Great work. Approved with one nit-pick.
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. |
felixarntz
left a comment
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
| 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.
felixarntz
left a comment
There was a problem hiding this comment.
Thanks @spacedmonkey, looks great! Good to commit IMO.
felixarntz
left a 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.