Drupal 10 compatibility JS aggregation causes js error with aggregation enabled

The assumption about this having something to do with jQuery is incorrect as H5P uses custom versions of everything, at least within internal iframes.

This is the issue of createCachedPublicFiles method of the H5PDrupal class.

This line will throw an error as the second argument is required (at least with PHP 8.1):

$cachedAsset = $optimizer->optimize($assets);

The logic tells that it should be changed to:

$cachedAsset = $optimizer->optimize($assets, []);

Which fixes the exception being thrown, but the end result is not what it is expected to be. This line will fail as there is no data key or it is empty, instead items are set and those point to the original files provided:

return array_map(function($publicUrl){ return \Drupal::service('file_url_generator')->generateAbsoluteString($publicUrl); }, array_column($cachedAsset, 'data'));

I'm not sure about the inner workings of the new JsCollectionOptimizerLazy class (or the one before that) and why this new lazy approach and creation of those files at the first request fails, but this is what is causing the issue. I've tried proving a library name and that gets the expected data key returned, but the files themselves are never created and the logger reports The libraries to include are encoded incorrectly.

My guess is that something has changed and the data provided for optimization is formatted incorrectly, though I see no documentation on how that should be formatted or what changed with the move to the new lazy approach.

The only really quick fix is to always disregard the JS and CSS preprocess configuration option, but that makes the whole optimization step meaningless. Just change the aggregateAssets method from this (this still keeps aggregation functional for everything else but the H5P):

  $jsAssetConfig = ['preprocess' => $systemPerformance->get('js.preprocess')];
  $cssAssetConfig = ['preprocess' => $systemPerformance->get('css.preprocess'), 'media' => 'css'];

to this:

  $jsAssetConfig = ['preprocess' => FALSE];
  $cssAssetConfig = ['preprocess' => FALSE, 'media' => 'css'];

I've checked the code and it seems that individual files are not really processed in any way, at least not with the type of file, unless at least one library is provided. This more or less means that the whole optimization code is useless, unless libraries are used, which is not really applicable in this case.

Not sure is this is the new internal logic or not, but providing any library prevents the aggregated file from being created. The other issue is that there is no cache busting logic present like it used to be with the previous approach and recreation of the combined file using fresh versions of the resources each time new cache file is generated.

It is possible to define services for old versions of both JS and CSS optimizer classes, though both are marked s deprecated and will be removed in version 11. Another way would be to create a custom solution based on previous versions of classes and use that instead, though that would have to get the cleanup procedures for unused versions of asset files and handling of handling of caches being cleared.

Long story short - new solution will not work with custom JS and CSS files. Declaring custom services based on now deprecated classes worked for JS part, yet styles were not loaded correctly (at least in my case). It seems that the only viable solution left is to create a custom optimizer logic that would just mimic the one that worked earlier and combine the file groups into one file, while keeping track of such files and removing the old versions.

It seems that new solution is purely based on libraries and does not really work on any of the custom resources like the previous one did. The new approach uses AssetResolver class that will be provided the names of the libraries and libraries only, extracted by the corresponding controller of the system module. Any custom assets are not really welcome by the optimizer and the behavior with not providing any libraries that does not process any of the custom individual files seems to be the correct one, as those are not considered for optimization or something along the lines.

It seems that a few modifications can help with getting the old way of dealing with assets running again. It requires redefining and using old versions of collection optimizer services. A few modifications to the current code would also be required as there are some changes in how new approach works, including changes to the dependencies (mostly affects the CSS, at least it was like that for me).

First, a h5p.services.yml file needs to be added, having copies of services from Drupal version 9.5 with a legacy suffix:

services:
  asset.css.collection_optimizer_legacy:
    class: Drupal\Core\Asset\CssCollectionOptimizer
    arguments: [ '@asset.css.collection_grouper', '@asset.css.optimizer', '@asset.css.dumper', '@state', '@file_system' ]
  asset.js.collection_optimizer_legacy:
    class: Drupal\Core\Asset\JsCollectionOptimizer
    arguments: [ '@asset.js.collection_grouper', '@asset.js.optimizer', '@asset.js.dumper', '@state', '@file_system' ]

Secondyl H5PDrupal.php file has to be patched with changes to aggregatedAssets and createCachedPublicFiles methods:

/**
   * Aggregate assets.
   *
   * @param array $scriptAssets
   *   JavaScript assets.
   * @param array $styleAssets
   *   Stylesheet assets.
   */
  public static function aggregatedAssets($scriptAssets, $styleAssets) {
    $jsOptimizer = \Drupal::service('asset.js.collection_optimizer_legacy'); // NB! Different service being used
    $cssOptimizer = \Drupal::service('asset.css.collection_optimizer_legacy'); // NB! Different service being used
    $systemPerformance = \Drupal::config('system.performance');
    $jsAssetConfig = ['preprocess' => $systemPerformance->get('js.preprocess')];
    $cssAssetConfig = ['preprocess' => $systemPerformance->get('css.preprocess'), 'media' => 'all']; // NB! Value of "media" changed from "css" to "all"
    $assets = ['scripts' => [], 'styles' => []];
    foreach ($scriptAssets as $jsFiles) {
      $assets['scripts'][] = self::createCachedPublicFiles($jsFiles, $jsOptimizer, $jsAssetConfig);
    }
    foreach ($styleAssets as $cssFiles) {
      $assets['styles'][] = self::createCachedPublicFiles($cssFiles, $cssOptimizer, $cssAssetConfig);
    }
    return $assets;
  }

  /**
   * Combines a set of files to a cached version, that is public available
   *
   * @param string[] $filePaths
   * @param AssetCollectionOptimizerInterface $optimizer
   * @param array $assetConfig
   *
   * @return string[]
   */
  private static function createCachedPublicFiles(array $filePaths, $optimizer, $assetConfig) {
    $assets = [];

    $defaultAssetConfig = [
      'type' => 'file',
      'group' => 'h5p',
      'cache' => TRUE,
      'attributes' => [],
      'version' => NULL,
      'browsers' => [],
      'license' => FALSE, // NB! Missing license key added to prevent missing key notices
    ];

    foreach ($filePaths as $index => $path) {
      $path = explode('?', $path)[0];

      $assets[$path] = [
        'weight' => count($filePaths) - $index,
        'data' => $path,
      ] + $assetConfig + $defaultAssetConfig;
    }
    $cachedAsset = $optimizer->optimize($assets, []);

    return array_map(function($publicUrl){ return \Drupal::service('file_url_generator')->generateAbsoluteString($publicUrl); }, array_column($cachedAsset, 'data'));
  }

This seems to be making everything work as expected, at least my testing material looks the way it used to and has no JS errors. It might be able to work until Drupal version 11.

The best approach would be to use the new logic and somehow dynamically define a library for each of the material, thus making the new lazy approach work as expected. One way would be to pass a library called h5p-content- and then dynamically define that library with the hook if the current context is one of the controllers for JS and CSS asset optimizers and the query string has some custom keys present. There might be a better approach, but this is what has come to my mind. Another possibility would be to mimic the new lazy approach and have a custom controllers that create optimized asset files or serve already existing ones.

Here is the patch that uses old versions of JS and CSS optimisation services with a few additional fixes for related problems of missing keys and warnings.

+1 RTBC I applied patch #6 with the Drupal 10 compatibility patch. It is functioning correctly.

Patch #6 failed with the latest D10 compatibility patch of h5p i.e
https://www.drupal.org/project/h5p/issues/3329297#comment-15218847

With the help of patch #6 created a new patch.
h5p version 2.0.0-alpha3
Tested on Drupal 10.1.5.

This #8 patch does not apply with the latest dev.
And with 2.0.0-alpha3 I am not able to get it working with D10 properly, cos I dont know which patches it is missinge compared to dev?

I can confirm too that, when using the latest dev version "drupal/h5p": "2.0.x-dev@dev", that most, but not all H5P creations DO NOT render when JavaScript aggregation is active.

By contrast to @Anaconda777's comment, mine DO all seem to work with CSS aggregation active; it is only the JavaScript aggregation.

Against the current dev version patch #6 almost works. The changes in src/H5PDrupal/H5PDrupal.php in line 88, 89 and 94 don't apply, I think. After I changed these lines manually, displaying the H5P content worked again. Patch #8 contains a lot of the changes to get H5P to work with Drupal 10, which are in some way already part of the dev version, soon to be tagged as alpha4.

These needs a re-roll.

Rerolled the patch from #8 by manual comparing what was required.

@googletrop Thanks, that patch applies but now when viewing H5P content I get wsod and this:

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "asset.js.collection_optimizer_legacy". Did you mean one of these: "asset.css.collection_optimizer", "asset.js.collection_optimizer"? i Drupal\Component\DependencyInjection\Container->get() (rad 157 av /var/www/html/drupal10/web/core/lib/Drupal/Component/DependencyInjection/Container.php).

I have Drupal 10.1.6

Without this patch there is not error message, it just does not show the H5P content when js aggregation is on

The reroll is missing the two legacy services added in the previous patches.

Now that #3329297: Automated Drupal 10 compatibility fixes has landed, the patch #6 should be re-rolled.

This is a reroll based on #8.

Committed/pushed to 2.0.x so that this change is available with the dev release. Thanks!

Actually no this isn't good either, it makes the H5P module only compatible with Drupal 10.1, not 9.5 or 10.0.x, because the legacy classes don't exist in those versions. Also using the legacy services is a stopgap anyway.

Two options:

1. Define the services in a service provider instead of YAML so it can check for the class existence first. Then in the actual logic, check the Drupal core version and only use the legacy services if we're on 10.1.x or higher. This adds more complexity and is still a stopgap.

2. Refactor this entirely to either not aggregate, or to not use the the Drupal core aggregation API at all and just concatenate the files together instead - or at least use more targeted bits of the API.

can confirm, patch #17 works with D10.1.6 and latest H5P dev

Thanks / kiitos

I was wrong in #20, because the 'legacy' classes are just the old one shipped with 9.5/10.0, then this approach works fine as a stopgap and should be compatible with both 9.5/10.0/10.1.

I opened #3401541: Refactor asset aggregation to avoid using deprecated asset optimization classes to refactor things in time for Drupal 11 compatibility though. Moving this issue back to fixed.