![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png){kind=logo}
#45346 closed defect (bug) (fixed)
Varnish querystring.sort can break load-styles.php
| Reported by: | rabin.io | Owned by: | azaozz |
|---|---|---|---|
| Priority: | normal | Milestone: | 5.3 |
| Component: | Script Loader | Version: | 4.9.8 |
| Severity: | normal | Keywords: | |
| Cc: | Focuses: |
Description
If one use Varnish with this snippet
# Sort the querystring parameters, so different orders of the same produce a single cache object.
if (req.url ~ "\?") {
set req.url = querystring.sort(req.url);
}
And if load-styles.php get an array for the load parameter and this parameter is sorted, it can break the page by not loading all the style elements.
e.g:
this url
/load-styles.php?c=0&dir=rtl&load[]=dashicons,admin-bar,common,forms,admin-menu,dashboard,list-tables,edit,revisions,media,themes,about,nav-menus,wp-pointer,widgets&load[]=,site-icon,l10n,buttons&ver=4.9.8
the array will look like this after sorting
[load] => Array
(
[0] => dashicons,admin-bar,common,forms,admin-menu,dashboard,list-tables,edit,revisions,media,themes,about,nav-menus,wp-pointer,widgets
[1] => ,site-icon,l10n,buttons
)
and the result will be,
Array
(
[0] =>
[1] => site-icon
[2] => l10n
[3] => buttonsdashicons <-------- this is the problem
[4] => admin-bar
[5] => common
[6] => forms
[7] => admin-menu
[8] => dashboard
[9] => list-tables
[10] => edit
[11] => revisions
[12] => media
[13] => themes
[14] => about
[15] => nav-menus
[16] => wp-pointer
[17] => widgets
)
the fix is very simple,
diff --git a/wp-admin/load-styles.php b/wp-admin/load-styles.php
index de20881..f45cfe2 100644
--- a/wp-admin/load-styles.php
+++ b/wp-admin/load-styles.php
@@ -20,7 +20,8 @@ require( ABSPATH . WPINC . '/version.php' );
$load = $_GET['load'];
if ( is_array( $load ) ) {
- $load = implode( '', $load );
+ $load = array_map( function ($item) { return trim($item, ','); }, $load );
+ $load = implode( ',', $load );
}
$load = preg_replace( '/[^a-z0-9,_-]+/i', '', $load );
$load = array_unique( explode( ',', $load ) );
but is it the right solution ?
Attachments (1)
Change History (13)
#2
@
8 years ago
Just a thought, right now WP pass the parameters with empty index
load[]=XXXXX&load[]=YYYY
what if when you split the string you assigned/pass then with index's
this way the sorting won't affect the order of the load array
load[1]=XXXXX&load[2]=YYY
#5
@
7 years ago
- Milestone Awaiting Review → 5.3
We need to maintain the order of loading of the scripts too. How about making this an associative array. That should prevent the sorting:
/load-styles.php?c=0&dir=rtl&load[chunk_0]=dashicons,admin-bar,common,forms,admin-menu,dashboard,list-tables,edit,revisions,media,themes,about,nav-menus,wp-pointer,widgets&load[chunk_1]=,site-icon,l10n,buttons&ver=4.9.8
Which will produce:
[load] => Array (
[chunk_0] => dashicons,admin-bar,common,forms,admin-menu,dashboard,list-tables,edit,revisions,media,themes,about,nav-menus,wp-pointer,widgets
[chunk_1] => ,site-icon,l10n,buttons
)
Can even do a ksort() in load-scripts.php for a good measure.
Patch coming up.
#7
@
7 years ago
In 45346.diff:
- Make the
loadarray associative so we can prevent sorting. - Use
ksort()inload-scripts.phpandload-styles.php.
@rabinio please test.
after 6 month of waiting...
Thinking it would have helped if there was a "testable" patch, especially with some unit tests. Of course you're welcome to add unit tests now if this patch works well :)
#8
@
7 years ago
It will take some time for me to validate this, as I don't have this setup in my dev environment right now, so if any one else can test it, that will help.
thx.
#9
@
7 years ago
Note: as discussed in https://core.trac.wordpress.org/ticket/26886#comment:39 the sorting function here set req.url = querystring.sort(req.url); should not sort (return early) if the query string contains an array, i.e. if more than one param name has [ and ] in it.
Related: #22757, #26886.