AssetResolver::getCssAssets $cid does not account for ckeditor5-stylesheets variations

This makes sense but it will need a decent inline comment explaining what's going on, could be based on the issue summary.

Uploading a patch file for reference.

Here's a start.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

samit.310@gmail.com → made their first commit to this issue’s fork.

This looks good to me. I thought about test coverage, but we'd have to create a testing module that dynamically sets the default theme alongside something like ckeditor5_stylesheets so that it actually breaks the logic, this seems very convoluted.

Or we'd just be asserting that the default theme is part of the cache ID, which doesn't seem very useful. It's really just a change to support contrib modules doing things that are complicated, but having the default theme in the core cache ID is harmless when they don't, cache hit rates will be the same as before.

So I think this is OK without test coverage and moving to RTBC.

Needs the backward compatibility dance in the constructor → .

@longwave, what would be the versions referenced within the message, 11.1.0 and 12.0.0?

    if ($theme_handler === NULL) {
      @trigger_error('Calling ' . __METHOD__ . ' without the $theme_handler argument is deprecated in drupal:11.1.0 and it will be required in drupal:12.0.0. See https://www.drupal.org/node/3078162', E_USER_DEPRECATED);
      $theme_handler = \Drupal::service('theme_handler');
    }

I'm guessing a change record is also required?

I attempted to add this change here using 11.1.0 and 12.0.0 as the versions and referencing this issue in the message.

bc layer looks good, that's all we need.

For 11.0.0 and lower backport, we could use a version of the MR without dependency injection that just calls \Drupal::service() to get the theme manager instead, that can happen before or after this is committed to 11.x

Observed test failures, found there are some PHP Coding standard issues due to which pipeline failed, addressed them, pipeline passed & MR is mergeable now. it's already in RTBC state , so keeping it to prev state (not sure about)

nod_ → closed merge request !8272

Committed 4deadaa and pushed to 11.x. Thanks!

Ported the changes to 11.0.X by creating MR 9347 & rebased MR as well it is several commits behind.

With 11.2 releasing soon not sure we need to backport this all the way to 11.0

Automatically closed - issue fixed for 2 weeks with no activity.