🇪🇪Estonia @pjotr.savitski

I've tagged release 1.2.0 that should support Drupal 11, but I've not tested it myself dues to reasons outlined above. I did run the phpstan and upgrade_status to maker sure that there is no deprecated code and by all accounts it should work. I'm closing the issue, but you can reopen it if it does not work with with latest 11.x.

Yes, but as H5P module itself is not compatible withe the latest and greatest, I would not be able to test if everything works as expected unless I am willing to put a lot work into the base module working.

I've looked at the code and it would most probably just require changes to the info.yml file to get it working.

If you are willing to test it, then I could just make the changes and push them to the main branch. Making a release if all is working as expected.

@abhinavk Using legacy/deprecated aggregator class was a quick and rather low effort solution to library code not working with the new aggregator class. I don't remember the inner details as I last looked at it more or less a year or so ago, but the methods aggregatedAssets and createCachedPublicFiles are the ones that are causing the issues.

H5P is not using libraries as Drupal generally does, but content type libraries specify static assets that are determined at runtime. Getting that to work with new cache system could boil down to a simple change, but I vaguely remember that it does make a new request to an endpoint and that one does no have the files that have to be optimized.

Here is an example of how it works taken from the field formatter:

• It determines H5P internal dependencies for the content
• Runs the alter hooks to allow modifying the resulting list
• Then aggregates the core assets and additional scripts and styles

Aggregates are overwritten on each save or content view and removed on each cache rebuild (probably not rebuilt until the content is accessed or updated). If I remember it correctly, new aggregation system works based on libraries and those are specified as a query string. That means that most of the files are not present as those have no libraries and thus are not added to the aggregator.

Maybe there is a way to define a library for each content type with some kind of hook allowing that library to be filled with assets using content type identifier as a basis. This means that the code determining the assets would be removed from the formatter and moved to that hook (most probably hook alter) that would deal with determining the assets and creating the resulting aggregates. One example would be h5p-content-123 where 123 is the unique identifier of the entity.

I have not dived deep into aggregate handler inner workings and know how deprecated one maps to the currently active solution. Thus I'm making several guesses and drawn conclusions could be totally wrong. One thing is certain - not using aggregation would mean that a lot of files are loaded at once as H5P uses internal dependency system that allows higher level content libraries to use lower level ones as building blocks. Content types like Course Presentation would probably be loading tens of scripts and several styles. Core also consists of several static asset files.

pjotr.savitski → created an issue.

I've just had the of restoring the phpunit.xml file after updating the core and it seems that a better solution might be to just put the configuration file outside of web/core and then change the path to the bootstrap file (Example: bootstrap="web/core/tests/bootstrap.php"). Running the code from project root where configuration file is situated should work as expected ./vendor/bin/phpunit --stop-on-fail /path/to/direcotry/or/test/file.

Using an IDE like PhpStorm allows setting location for both files, but just changing the path would do the trick.

@himanshu_jhaloya The fix is already part of the code in the repository and the issue is that a new release has to be made by the project maintainers.

pjotr.savitski → created an issue.

It seems that there is an issue with the current code that prevents a certain field form being targeted.

The issue is with the !empty($error->arrayPropertyPath) check of the errorElement method that will return false regardless of the real value of arrayPropertyPath property that seems to be fetched through the magic __get method. The solution could be either to define a local variable and use that instead or use the getPropertyPath method instead.

The resulting code would be either

  public function errorElement(array $element, ConstraintViolationInterface $error, array $form, FormStateInterface $form_state) {
    // Validation errors might be a about a specific (behavior) form element
    // attempt to find a matching element.
    $path = $error->arrayPropertyPath;
    if (!empty($path) && $sub_element = NestedArray::getValue($element, $path)) {
      return $sub_element;
    }
    return $element;
  }

or

  public function errorElement(array $element, ConstraintViolationInterface $error, array $form, FormStateInterface $form_state) {
    // Validation errors might be a about a specific (behavior) form element
    // attempt to find a matching element.
    if (!empty($error->getPropertyPath()) && $sub_element = NestedArray::getValue($element, $error->arrayPropertyPath)) {
      return $sub_element;
    }
    return $element;
  }

pjotr.savitski → changed the visibility of the branch 3420414-warning-count-parameter to hidden.

pjotr.savitski → created an issue.

Here is the comparison between the released version tag and the main branch. The new version has not been released and that is it.

I've uploaded bfd-drupal-10-support.patch file as it has a few additional changes that made the Upgrade Status → module happy. Those include changes to a few more templates that are not really used, unless content types are present.

The main difference is the change to the global-styling library that removes core/popperjs and core/jquery-once. The first one is solved by using a bundle version of Bootstrap that includes the dependency, while the latter one does not seem to be used by the theme JS file at all.

One additional change is to get rid of Drupal 8 support because that version has already been discontinued a long time ago. To be honest, a later release could remove the Drupal 9 support. I've kept it to be able to make sure that everything is working as expected while still using version 9 and then migrate to 10.

Community versions of Classy and Stable themes seem to be doing the trick for Drupal 10, although not having those required would be a plus and a possibility to further migrate to version 11.

This will most probably never be accepted as this is a custom field that is not part of Drupal Core (especially with custom field being added through the UI). You should instead create a new custom module or modify an existing one that would implement the API hook and include the required field. There is no real reason to modify the original module.

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.

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.

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.

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'm not yet 100% sure, but inability to use interactive items might have everything to do with removal of the calls to jquery.once and not replacing those with the new custom once that is available in both versions 9 and 10.

H5P is not using the default CKEditor added by Drupal, but a custom one that is shipped with the h5p-editor dependency. This is why ckeditor/ckeditor.js is a false positive and irrelevant to the upgrade process.

I've currently refactored the code in application.js file to use the new logic and the detachment code looks like this (I'm not including the longer one from attachment function):

detach: function (context, settings, trigger) {
      if (trigger === 'serialize') {
        once('H5PEditor', '.h5p-editor-iframe', context).forEach(function(element) {
          for (var i = 0; i < submitHandlers.length; i++) {
            if (submitHandlers[i].element === element) {
              // Trigger submit handler
              submitHandlers[i].handler();
            }
          }
        });
      }
    }

It might be better to use the

use Drupal\Component\EventDispatcher\Event;

for FinishedEvent class as that would work with both Drupal 9 and 10.