-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Register block style early. #4621
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple driveby nits
Co-authored-by: Weston Ruter <westonruter@google.com>
CC @aristath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spacedmonkey This looks great so far. Only a bit of feedback below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @spacedmonkey, Left some feedback.
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spacedmonkey A few last feedback, but almost good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spacedmonkey Left some feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @spacedmonkey for the update. It look good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete_transient( 'wp_core_block_styles' ); | |
delete_transient( 'wp_core_block_css_files' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @spacedmonkey, looks great! Good to commit IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.