-
Notifications
You must be signed in to change notification settings - Fork 98
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
Add module to alert about excessive JS and CSS assets #54
Add module to alert about excessive JS and CSS assets #54
Conversation
…blob/master/site-health-audit-enqueued-assets.php imported as a module and some little refactoring.
…tivated_plugin, deactivated_plugin
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 for working on this 😄 I see this is a draft but I left a few early comments without doing a detailed review yet. The approach to trigger an asset warning is quite weak at the moment and need some more work.
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 so far @manuelRod! I agree the most important decision we need to make here is find good thresholds for the amount of assets and their filesize when we should start warning users.
Other than that, I left a few specific code comments below for things I think can be improved. My most important point would be that we also need to add appropriate capability checks and collect the enqueued assets data in a single consistent scenario instead of "anywhere in the frontend".
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.
@manuelRod Two higher-level points of feedback:
- Now that Module Slugs Naming Proposal #46 / Update module directories to be within their focus directory #58 is merged, please refresh this against latest
trunk
and update all the module- and test-related files here to be in asite-health
subdirectory, according to the updated module specification. - It may help to split the module itself into a few smaller files, right now there is just a single
load.php
which makes it hard to overview the code. A good starting point could be to have only the main hooks and their functions in theload.php
file and the other lower-level functions could be in separate files that arerequire_once
d inload.php
.
…s and unit test reordering.
- renaming transients - view_site_health_checks capability check to avoid non logged and non view_site_health_checks users to trigger checks - is_front_page check to avoid creating warnings in any random page.
@felixarntz I've implemented all your requests and updated the module to the new structure, let's continue the conversation in the issue for thresholds and messaging. |
@JustinyAhin Would you mind explaining what would be the issue? |
@manuelRod the query monitor CSS is only enqueued when logged in as an admin. I was wondering if we should include these kinds of assets in the checks. |
Maybe we could have a filter to allow changing the list of scripts/styles that the implementation detects? |
@JustinyAhin @aristath I added the check of capabilities to save transients per @felixarntz request here.
|
Hmmm I don't see how setting a transient could be considered a security risk... 🤔 What if we added a button in the module, something like "refresh stats/caches/whatever"? On click, we could load the frontend in an invisible iframe using a nonce & a special param in the URL to trigger the transient update. In the request, we can also "fake" the user-ID to be an anonymous user so it doesn't load all the extra scripts/styles normally associated with an admin user. |
@aristath there is already a "reset" button to clean the transient (but that's about it). |
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.
@manuelRod First of all, apologies for the long delay in review - I'll make sure to follow up more quickly next time.
This is mostly looking good now, I left a few more comments, but after those are addressed, LGTM from my end.
* @return string Returns absolute path to the resource. | ||
*/ | ||
function perflab_aea_get_path_from_resource_url( $resource_url ) { | ||
return ABSPATH . wp_make_link_relative( $resource_url ); |
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.
This is okay for a start, but it's a bit oversimplified:
- It is possible for URLs to be in a location that is not within
ABSPATH
, for example several sites have theirwp-content
directory separate, which is supported by WordPress. - WordPress may be in a subdirectory of the domain.
For most sites what you have here will work, but we should make sure to also accomodate those other sites.
I suggest we expand this as follows to at least cover for these two cases a bit better:
return ABSPATH . wp_make_link_relative( $resource_url ); | |
if ( 0 === strpos( $resource_url, WP_CONTENT_URL ) ) { | |
return WP_CONTENT_DIR . substr( $resource_url, strlen( WP_CONTENT_URL ) ); | |
} | |
$site_url = untrailingslashit( site_url() ); | |
if ( 0 === strpos( $resource_url, $site_url ) ) { | |
return ABSPATH . substr( $resource_url, strlen( $site_url ) ); | |
} | |
return ABSPATH . wp_make_link_relative( $resource_url ); |
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.
@felixarntz can you give me an example of the first case? I cannot see it clearly.
Regarding WordPress may be in a subdirectory of the domain.
Do you mean something like this?
https://example.com/blog/wp-content/themes/test-theme/style.css ?
If that's the case, doesn't ABSPATH . wp_make_link_relative( $resource_url ) covers it too?
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.
@manuelRod Some popular environments like https://github.com/roots/bedrock have their wp-content
directory outside of the (rest of the) WordPress directory, so that URLs would be e.g. https://example.com/content/themes/...
and https://example.com/wp/wp-includes/...
. Both conditions I'm suggesting here cover that use-case, the first is for assets that come from the wp-content
directory in a custom location, the second one for when the WordPress directory is in a subdirectory.
If that's the case, doesn't ABSPATH . wp_make_link_relative( $resource_url ) covers it too?
I don't think it does, since wp_make_link_relative
simply removes the protocol and domain. Looking at my example above, that means you get /wp/wp-includes/...
, and when you append that to ABSPATH
, the wp
directory would basically be in there twice. That's why for these cases we have to use the site_url()
function, which is the "URL equivalent" of ABSPATH
(the same way that WP_CONTENT_URL
is the "URL equivalent" to WP_CONTENT_DIR
).
@manuelRod @aristath @JustinyAhin How do you feel about merging this PR once it's good to go with what there is for now? This is a fair amount of code, and once the last quirks are resolved, I think this is a good first proof of concept. While in WP core we only commit things we really think are ready, this makes code review very complicated and unfocused, and I don't think we need to operate like that for this plugin (it's not the case in Gutenberg either). I agree that we definitely have to find a solution for excluding the admin-exclusive assets for the numbers here, but I think we can explore that in a follow-up issue that is only focused on that and not with everything else in this module. Same goes for the thresholds which we may still iterate on. How does that sound to you? |
I agree, as a POC this is fine. I'm good with merging this one as-is, and then iterate and improve things as we go 👍 |
Agree, I think this version is a good starting point and we can iterate it later. Also good to have it shipped with beta v1.0.0 so this plugin gets more stuff and exposure. @felixarntz thanks for the new feedback, I'll check |
@felixarntz I also agree on merging this as it is now, and iterate after that. |
@felixarntz there is only one outstanding point, see after this we can merge! |
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.
@manuelRod Thanks for all the updates! This is pretty much good to go from my perspective, I just left some super minor nit-pick comments on things that would be great to address, but not critical. The only other thing I think needs to be addressed still is #54 (comment).
Would love for someone else to review this as well - any volunteers? :)
tests/testdata/modules/site-health/audit-enqueued-assets/class-site-health-mock-responses.php
Outdated
Show resolved
Hide resolved
tests/testdata/modules/site-health/audit-enqueued-assets/class-site-health-mock-responses.php
Outdated
Show resolved
Hide resolved
tests/testdata/modules/site-health/audit-enqueued-assets/class-site-health-mock-responses.php
Outdated
Show resolved
Hide resolved
tests/testdata/modules/site-health/audit-enqueued-assets/class-site-health-mock-responses.php
Outdated
Show resolved
Hide resolved
function perflab_aea_audit_enqueued_styles() { | ||
if ( ! is_admin() && is_front_page() && current_user_can( 'view_site_health_checks' ) && false === get_transient( 'aea_enqueued_front_page_styles' ) ) { | ||
global $wp_styles; | ||
$enqueued_styles = array(); | ||
foreach ( $wp_styles->queue as $handle ) { | ||
$src = $wp_styles->registered[ $handle ]->src; | ||
if ( $src && ! strpos( $src, 'wp-includes' ) ) { | ||
$enqueued_styles[] = array( | ||
'src' => $src, | ||
'size' => perflab_aea_get_resource_file_size( perflab_aea_get_path_from_resource_url( $src ) ), | ||
); | ||
} | ||
} | ||
set_transient( 'aea_enqueued_front_page_styles', $enqueued_styles, 12 * HOUR_IN_SECONDS ); | ||
} | ||
} | ||
add_action( 'wp_print_styles', 'perflab_aea_audit_enqueued_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.
A few notes for this one:
- Styles can be printed in the header or the footer. I'm not sure
wp_print_styles
would cover all scenarios - Stylesheets can have additional styles added inline - which should also be accounted for when getting the size
- For some styles we already know their path (it was part of a refactor for block styles which was merged in 5.9)
$wp_styles->queue
contains styles that are to be enqueued. However, a stylesheet can be enqueued after this has run, and if I'm not mistaken any styles that have already been printed will not be included here. To avoid a priorities battle and over-complicaing the implementation, it would be better if we ran this as the last action inwp_footer
, and check which stylesheets were done.
The below code fixes the above issues:
add_action( 'wp_footer', function() {
global $wp_styles;
$enqueued_styles = array();
// Loop through the styles that were printed, and get their details.
foreach ( $wp_styles->done as $style_handle ) {
$style = $wp_styles->registered[ $style_handle ];
if ( ! empty( $style->extra ) && ! empty( $style->extra['path'] ) ) { // Check if we already have the style's path.
$path = $style->extra['path'];
} else { // Fallback to getting the path from the style's src.
$path = perflab_aea_get_path_from_resource_url( $style->src );
}
// Add any extra data that was passed with the style.
$extras_size = 0;
if ( ! empty( $style->extra ) && ! empty( $style->extra['after'] ) ) {
foreach ( $style->extra['after'] as $extra ) {
$extras_size += strlen( $extra );
}
}
$enqueued_styles[] = array(
'src' => $style->src,
'size' => perflab_aea_get_resource_file_size( $src ) + $extras_size,
);
}
set_transient( 'aea_enqueued_front_page_styles', $enqueued_styles, 12 * HOUR_IN_SECONDS );
}, PHP_INT_MAX );
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, @aristath for this valuable feedback, would this apply also to scripts? or just to 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 apply also to scripts? or just to styles?
Some (but not all) parts also apply to scripts, yes. Scripts can get added in the head or the footer, and they can get added while the page renders. For example, a block can enqueue its own script on-render. This will make the script get added only when the block gets rendered, but it won't be visible to $wp_scripts
before we get to the footer.
However, scripts don't (yet) have a path
defined, this is something we plan on doing in WP 6.0 - so that part can be skipped 👍
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.
Good catch @aristath! Since considering this is also adding an extra "dimension" of code to this though, based on my earlier comment, would it be okay to enhance/fix this in a follow-up issue and PR?
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.
Of course! We can merge this one as-is, and improve in follow-up PRs 👍
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.
Approved from my end - awesome work so far @manuelRod!
Per some of the comments, there are definitely a number of things to follow up on - the most important one from my personal perspective is collecting the asset information in a way that doesn't include admin-specific assets.
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.
Approving this one as it's a good starting point and we can iterate as we go in followup PRs 👍
💯 |
Addresses #25