Compress aggregate URL query strings

We can also add a new data attribute on the script tags that have this information in clear format so we don't need to mess around with url parsing.

I think it'd be that simple. We don't use it in the frontend, it's only to send back to php.

on umami frontpage with this URL:

js_PEdSRDCYlyswo_e1QJZX-ZKulvo_gfga7JWEGxn4yMU.js?scope=footer&delta=0&language=en&theme=umami&include=core/jquery%2Ccore/once%2Ctour/tour%2Ccore/drupal.progress%2Ccore/tabbable%2Ccore/loadjs%2Cresponsive_image/ajax%2Ccore/drupal.ajax%2Ccontextual/drupal.contextual-links%2Csystem/base%2Cumami/classy.base%2Ccore/normalize%2Cumami/demo-umami-tour%2Cumami/global%2Cumami/messages%2Cumami/webfonts-open-sans%2Cumami/webfonts-scope-one%2Ccore/drupal.active-link%2Cviews/views.module%2Cumami/recipe-collections%2Cumami/more-link%2Cumami/view-mode-card-common%2Cumami/view-mode-card%2Cumami/classy.node%2Cumami/view-mode-card-common-alt%2Ccore/modernizr%2Ccore/drupal.debounce%2Ctoolbar/toolbar%2Cuser/drupal.user.icons%2Ccore/drupal.tabbingmanager%2Ccontextual/drupal.contextual-toolbar%2Cshortcut/drupal.shortcut%2Ctoolbar/toolbar.escapeAdmin%2Cbig_pipe/big_pipe

• Core: 860
• patch #4: 528
• patch #5: 575
• patch #6: 553
• Using ~ as a separator: 794

Not too happy about having the language, theme, include, exclude params as a set indexes of the data array, feels like a future headache.

One thing to be aware of is that using gzcompress makes it impossible to use from the JS (at least without using something like zlib which adds quite a bit of js just for that).

One thing we could do easily is replace the , separator by ~ because that one isn't urlencoded so we save 2chars for each separator, which can add up as seen above. I haven't seen a ~ in a library name declaration so that should be safe enough, we could always prevent people from using that character in the library definition.

1. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -62,6 +62,47 @@ public static function buildQuery(array $query, $parent = '') {
   +    // Use json_encode() instead of serialize(), because this is used to
   +    // compress/uncompress user data.
   

   That comment is either unnecessary or in the wrong place.

2. +++ b/core/modules/system/src/Controller/AssetControllerBase.php
   @@ -147,9 +151,19 @@ public function deliver(Request $request, string $file_name) {
   +      $attached_assets->setAlreadyLoadedLibraries(explode(',', UrlHelper::uncompressQueryParameter($request->query->get('exclude'))));
   

   could be explode(',', $exclude_string) here no?

3. +++ b/core/tests/Drupal/FunctionalTests/Asset/AssetOptimizationTest.php
   @@ -164,19 +173,21 @@ protected function replaceGroupHash(string $url): string {
   +    $include = explode(',', UrlHelper::uncompressQueryParameter($parts['query']['include']));
   +    $includes[] = 'system/llama';
   +    $parts['query']['include'] = UrlHelper::compressQueryParameter(implode(',', $include));
   

   that should be $include[] here no? (without the "s")

Still at 528 chars, all good. It makes sense that only compressing the include and exclude value the string would be the shortest since there are a lot or repetition inside that string "core/", "core/drupal.", "umami/", etc. so it checks out that it's the most efficient.

Tried to replace "," by "~", and the string ends up longer at 540 chars. So no need to open a follow-up for that since it never appear in the URL.

This will mean that if we use that include (and exclude) parameters for ajaxPageState, the backend will need to be able to receive a list of encoded arguments instead of the exhaustive list of libraries on the page. Not relevant to this issue and not a big deal in itself, something to keep in mind for BC and all that.

Given all that it's RTBC for me.

1. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -62,6 +62,45 @@ public static function buildQuery(array $query, $parent = '') {
   +   * While RFC 1738 doesn't specify a maximum length for query strings,
   +   * browsers (such as Internet Explorer) or server configurations may
   +   * restrict URLs and/or query strings to a certain length, often 1000
   +   * or 2000 characters. This method can be used to compress a string into a
   +   * URL-safe query string which will be shorter than if it was used directly.
   

   🙈 Nit: 80 cols formatting

2. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -62,6 +62,45 @@ public static function buildQuery(array $query, $parent = '') {
   +   *   The uncompressed data or FALSE on failure.
   +   */
   +  public static function uncompressQueryParameter(string $compressed): string|bool {
   

   🤓 I was confused by "uncompress" — never seen that before! So did some searching and … apparently "uncompress" is rarely used, "uncompressed" is: to indicate that something is not/never was compressed.

   I think one usually uses "decompress" — just like "encode & decode".

   Sorry 😬

3. +++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizerLazy.php
   @@ -102,18 +115,9 @@ public function optimize(array $css_assets, array $libraries) {
   -    $ajax_page_state = $this->requestStack->getCurrentRequest()->get('ajax_page_state');
   -    $already_loaded = isset($ajax_page_state) ? explode(',', $ajax_page_state['libraries']) : [];
   -    $query_args = [
   

   🤔 Why did these get moved to a different location in the code?

4. +++ b/core/tests/Drupal/FunctionalTests/Asset/AssetOptimizationTest.php
   @@ -200,4 +211,58 @@ protected function omitTheme(string $url): string {
   +   *   The URL with the 'include' set to an arbitrary string.
   

   s/include/exclude/

5. +++ b/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php
   @@ -106,6 +106,25 @@ public function providerTestInvalidAbsolute() {
   +    $this->assertEquals($data, $uncompressed);
   +    $this->assertLessThan(strlen($uncompressed), strlen($compressed));
   

   Wow, super elegant test coverage! 🤩

6. +++ b/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php
   @@ -106,6 +106,25 @@ public function providerTestInvalidAbsolute() {
   +    // Pass an invalid string to ::uncompressQueryParameter() and ensure it
   +    // doesn't result in a PHP warning.
   +    $this->assertFalse(UrlHelper::uncompressQueryParameter('llama'));
   

   🤓 Nit: this feels like it belongs in a separate test method.

Hah! That's more than good enough for me 🤓

Patch still applies and works. Review from #21 addressed.

All good :)

1. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -62,6 +62,45 @@ public static function buildQuery(array $query, $parent = '') {
   +   * @see \Drupal\Component\Utility\UrlHelper::uncompress()
   

   @see uncompressQueryParameter()

2. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -62,6 +62,45 @@ public static function buildQuery(array $query, $parent = '') {
   +    return urlencode(base64_encode(gzcompress($data)));
   

   It seems to double encode unsafe characters from base64 encode (+, / and =).

   <script src="/sites/default/files/js/js_gi8MaXv8i2qj2W0p7yMIoHc3LqEQ2PXt8dRkQm4-wPg.js?scope=footer&amp;delta=0&amp;language=en&amp;theme=olivero&amp;include=eJxtjEEOwyAMBD8UypsMdolVgyObgvh9pB7oJZc97MyuL%252B9UYwKnQ4UHmcYimkCC9yXcyq4vnWSEIa2QRPNngzcRHoNpevzlqyp%252B5f%252FXYHCBztqCU9aGYGtDJ7B8hsn4OLiM64PewEznDVS%252FSdg%253D"></script>
   

   With "base64url" encoding the parameter would be few characters shorter. Something like this: https://www.php.net/manual/en/function.base64-encode.php#123098 ?

3. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -62,6 +62,45 @@ public static function buildQuery(array $query, $parent = '') {
   +   * @see \Drupal\Component\Utility\UrlHelper::compress()
   

   @see compressQueryParameter()

4. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -62,6 +62,45 @@ public static function buildQuery(array $query, $parent = '') {
   +    return @gzuncompress(base64_decode(urldecode($compressed)));
   

   do we need to add ext-zlib to composer.json for gz-functions?

Zlib PHP extension can't be disabled, no need to change composer

1. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -62,6 +62,48 @@ public static function buildQuery(array $query, $parent = '') {
   +   * browsers (such as Internet Explorer) or server configurations may restrict
   

   Nit: I don't think we support the Internet Explorer anymore? 🤓 (Also: 🥳)

2. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -62,6 +62,48 @@ public static function buildQuery(array $query, $parent = '') {
   +    return str_replace(['+', '/', '='], ['-', '_', ''], base64_encode(gzcompress($data)));
   ...
   +    return @gzuncompress(base64_decode(str_replace(['-', '_'], ['+', '/'], $compressed)));
   

   Fascinating that we can uncompress even though = was omitted?! 🤯

3. +++ b/core/modules/system/src/Controller/AssetControllerBase.php
   @@ -147,9 +151,19 @@ public function deliver(Request $request, string $file_name) {
   +    if (!$include_string) {
   +      throw new BadRequestHttpException('The libraries to include must be passed as a query argument');
   +    }
   

   🤔 This looks like the message for !$query->has('include'), but the message for ::uncompressQuery() returning FALSE?

   EDIT: ah, yes, the "has" check already happened above. We just need to tweak this message :)

4. What length does this result in compared to @nod_'s analysis in #10?

some libraries in umami changed so went back to the same commit i used in #10 to have numbers we can compare, with latest patch we're at 502, we have a winner.

For reference, using commit 6a1855c2:

• Core: 860
• patch #4: 528
• patch #5: 575
• patch #6: 553
• Using ~ as a separator: 794
• patch #30: 502

Seems like we're good to go now.

#33: TIL = is used for right-padding data in base 64 encoding! I actually always wondered what that was for, and never looked into it! 😅

Thanks for teaching me something! 🙏

#32: very nice!

RTBC++

Just realized one last thing 😬

What happens when you have cached responses (in Dynamic Page Cache, Page Cache, Varnish, CDN … — any reverse proxy) and that still contains uncompressed aggregate URL query strings? Does that still work?

Oh, HAH! #1014086: Stampedes and cold cache performance issues with css/js aggregation is 10.1.x-only, so … no existing sites are on this yet! And when updating to a new minor, caches are cleared already anyway 👍

+++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizerLazy.php
@@ -74,6 +74,19 @@ public function optimize(array $css_assets, array $libraries) {
+    // All asset group URLs will have exactly the same query arguments, except
+    // for the delta, so prepare them in advance.
+    $query_args = [
+      'language' => $this->languageManager->getCurrentLanguage()->getId(),
+      'theme' => $this->themeManager->getActiveTheme()->getName(),
+      'include' => UrlHelper::compressQueryParameter(implode(',', $this->dependencyResolver->getMinimalRepresentativeSubset($libraries))),
+    ];
+    $ajax_page_state = $this->requestStack->getCurrentRequest()->get('ajax_page_state');
+    $already_loaded = isset($ajax_page_state) ? explode(',', $ajax_page_state['libraries']) : [];
+    if ($already_loaded) {
+      $query_args['exclude'] = UrlHelper::compressQueryParameter(implode(',', $this->dependencyResolver->getMinimalRepresentativeSubset($already_loaded)));
+    }
+
     $css_assets = [];
     foreach ($css_groups as $order => $css_group) {
       // We have to return a single asset, not a group of assets. It is now up
@@ -102,18 +115,9 @@ public function optimize(array $css_assets, array $libraries) {

@@ -102,18 +115,9 @@ public function optimize(array $css_assets, array $libraries) {
         $css_assets[$order]['data'] = $uri;
       }
     }
+
     // Generate a URL for each group of assets, but do not process them inline,
     // this is done using optimizeGroup() when the asset path is requested.
-    $ajax_page_state = $this->requestStack->getCurrentRequest()->get('ajax_page_state');
-    $already_loaded = isset($ajax_page_state) ? explode(',', $ajax_page_state['libraries']) : [];
-    $query_args = [
-      'language' => $this->languageManager->getCurrentLanguage()->getId(),
-      'theme' => $this->themeManager->getActiveTheme()->getName(),
-      'include' => implode(',', $this->dependencyResolver->getMinimalRepresentativeSubset($libraries)),
-    ];
-    if ($already_loaded) {
-      $query_args['exclude'] = implode(',', $this->dependencyResolver->getMinimalRepresentativeSubset($already_loaded));
-    }

--- a/core/lib/Drupal/Core/Asset/JsCollectionOptimizerLazy.php
+++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizerLazy.php

+++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizerLazy.php
@@ -71,6 +71,21 @@ public function optimize(array $js_assets, array $libraries) {
+    // All group URLs have the same query arguments apart from the delta and
+    // scope, so prepare them in advance.
+    $language = $this->languageManager->getCurrentLanguage()->getId();
+    $query_args = [
+      'language' => $language,
+      'theme' => $this->themeManager->getActiveTheme()->getName(),
+      'include' => UrlHelper::compressQueryParameter(implode(',', $this->dependencyResolver->getMinimalRepresentativeSubset($libraries))),
+    ];
+    $ajax_page_state = $this->requestStack->getCurrentRequest()
+      ->get('ajax_page_state');
+    $already_loaded = isset($ajax_page_state) ? explode(',', $ajax_page_state['libraries']) : [];
+    if ($already_loaded) {
+      $query_args['exclude'] = UrlHelper::compressQueryParameter(implode(',', $this->dependencyResolver->getMinimalRepresentativeSubset($already_loaded)));
+    }
+
     // Group the assets.
     $js_groups = $this->grouper->group($js_assets);
 
@@ -109,33 +124,19 @@ public function optimize(array $js_assets, array $libraries) {

@@ -109,33 +124,19 @@ public function optimize(array $js_assets, array $libraries) {
           break;
       }
     }
-    if ($libraries) {
-      // Generate a URL for the group, but do not process it inline, this is
-      // done by \Drupal\system\controller\JsAssetController.
-      $ajax_page_state = $this->requestStack->getCurrentRequest()
-        ->get('ajax_page_state');
-      $already_loaded = isset($ajax_page_state) ? explode(',', $ajax_page_state['libraries']) : [];
-      $language = $this->languageManager->getCurrentLanguage()->getId();
-      $query_args = [
-        'language' => $language,
-        'theme' => $this->themeManager->getActiveTheme()->getName(),
-        'include' => implode(',', $this->dependencyResolver->getMinimalRepresentativeSubset($libraries)),
-      ];
-      if ($already_loaded) {
-        $query_args['exclude'] = implode(',', $this->dependencyResolver->getMinimalRepresentativeSubset($already_loaded));
-      }
-      foreach ($js_assets as $order => $js_asset) {
-        if (!empty($js_asset['preprocessed'])) {
-          $query = [
-            'scope' => $js_asset['scope'] === 'header' ? 'header' : 'footer',
-            'delta' => "$order",
-          ] + $query_args;
-          $filename = 'js_' . $this->generateHash($js_asset) . '.js';
-          $uri = 'public://js/' . $filename;
-          $js_assets[$order]['data'] = $this->fileUrlGenerator->generateAbsoluteString($uri) . '?' . UrlHelper::buildQuery($query);
-        }
-        unset($js_assets[$order]['items']);
+    // Generate a URL for the group, but do not process it inline, this is
+    // done by \Drupal\system\controller\JsAssetController.
+    foreach ($js_assets as $order => $js_asset) {
+      if (!empty($js_asset['preprocessed'])) {
+        $query = [
+          'scope' => $js_asset['scope'] === 'header' ? 'header' : 'footer',
+          'delta' => "$order",
+        ] + $query_args;
+        $filename = 'js_' . $this->generateHash($js_asset) . '.js';
+        $uri = 'public://js/' . $filename;
+        $js_assets[$order]['data'] = $this->fileUrlGenerator->generateAbsoluteString($uri) . '?' . UrlHelper::buildQuery($query);
       }
+      unset($js_assets[$order]['items']);
     }

I don't think we need to move the code blocks around. I think we can add UrlHelper::compressQueryParameter() where it is needed and not move all the code and change the flow. Just in case someone where is relying on the if ($libraries) { behaviour ... and trying to keep changes to minimal necessary to implement.

1. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -62,6 +62,49 @@ public static function buildQuery(array $query, $parent = '') {
   +    return str_replace(['+', '/', '='], ['-', '_', ''], base64_encode(gzcompress($data)));
   

   The double encoding is gone now.

   <script src="/sites/default/files/js/js_gi8MaXv8i2qj2W0p7yMIoHc3LqEQ2PXt8dRkQm4-wPg.js?scope=footer&amp;delta=0&amp;language=en&amp;theme=olivero&amp;include=eJxtjEEOwyAMBD8UypsMdolVgyObgvh9pB7oJZc97MyuL-9UYwKnQ4UHmcYimkCC9yXcyq4vnWSEIa2QRPNngzcRHoNpevzlqyp-5f_XYHCBztqCU9aGYGtDJ7B8hsn4OLiM64PewEznDVS_Sdg"></script>
   

2. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -62,6 +62,49 @@ public static function buildQuery(array $query, $parent = '') {
   +   * Takes a compressed, urlencoded string and converts it back to the original.
   

   Drop "urlencoded" or replace it with "URL-safe" or "Takes a compressed query parameter and converts it back to the original."?

3. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -62,6 +62,49 @@ public static function buildQuery(array $query, $parent = '') {
   +   * @see \Drupal\Component\Utility\UrlHelper::compress()
   

   @see \Drupal\Component\Utility\UrlHelper::compressQueryParameter()

Other than that looks good to me.

#42 looks good and addresses #41, working as expected for me and tests are passing. I don't see anything else here. RTBC

Committed 6d62cf2 and pushed to 10.1.x. Thanks!

The new code has test coverage and the URLs are smaller - nice.

YAY!

Hopefully see y'all in #3303067: Compress aggregate URL query strings next 🤓

#1945262: Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering

@voleger, oops, yes, that's what I meant 😅