Problem/Motivation

\Drupal\Core\Asset\AssetResolver::getCssAssets() computes the CSS assets to serve for the given AttachedAssetsInterface $assets, $optimize, LanguageInterface $language = NULL. The cache ID it uses is

$cid = 'css:' . $theme_info->getName() . ':' . $language->getId() . Crypt::hashBase64(serialize($libraries_to_load)) . (int) $optimize;

Its logic does

    if ($optimize) {
      $css = \Drupal::service('asset.css.collection_optimizer')->optimize($css, $libraries_to_load, $language);
    }
    $this->cache->set($cid, $css, CacheBackendInterface::CACHE_PERMANENT, ['library_info']);

That definitely also does not vary by url.site cache context.

Yet in case $optimize === TRUE, it generates absolute file URLs. This means it will serve the CSS assets for every site in a single-site-with-multiple-domains situation from whichever domain generated this CSS aggregate first!

The root cause: the above results in a call to \Drupal\Core\Asset\CssCollectionOptimizerLazy::optimize() (introduced by #1014086: Stampedes and cold cache performance issues with css/js aggregation) which does:

        $css_assets[$order]['data'] = $this->fileUrlGenerator->generateAbsoluteString($uri) . '?' . UrlHelper::buildQuery($query);

and indeed generates absolute URLs:

This is the regression.

I discovered it while updating the CDN contrib module for Drupal 10.1 in #3347181: 10.1.x compatibility: tests are failing against Drupal 10.1.x due to upstream changes, and for a reason I could not figure out, the CDN module wouldn't rewrite CSS aggregates to be served from a CDN … because Drupal core stopped generating root-relative asset URLs since #1014086: Stampedes and cold cache performance issues with css/js aggregation due to the above bug:

Steps to reproduce

Proposed resolution

Generate relative URLs.

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

Status: Needs work » Needs review
StatusFileSize
new1.85 KB
wim leers’s picture

This hard-blocks the CDN module from becoming compatible with Drupal 10.1: #3347181: 10.1.x compatibility: tests are failing against Drupal 10.1.x due to upstream changes.

catch’s picture

This is the same as #3354211: Tidy up URL generation for asset aggregates although the approach is different.

catch’s picture

Version: 11.0.x-dev » 10.1.x-dev
Status: Needs review » Reviewed & tested by the community

Since this avoids the urldecode() workaround I think we should go ahead here, the other issue can stay open for trying to use the asset uri directly and adding test coverage.

quietone’s picture

Title: Regression since #1014086: generated CSS assets have absolute URLs without varying by url.site cache context » [regression] Since #1014086 generated CSS assets have absolute URLs without varying by url.site cache context

Just changing title per Special titles.

  • catch committed 1021a1f1 on 10.1.x
    Issue #3354204 by Wim Leers: [regression] Since #1014086 generated CSS...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Since this is blocking and trivial, going ahead and committing to 10.1.x, thanks! We've got a follow-up for further refactoring and test coverage already in the issue I opened alongside this one.

wim leers’s picture

#3347181-13: 10.1.x compatibility: tests are failing against Drupal 10.1.x due to upstream changes is green on 10.1! 🚀 So I can confirm that contrib is unblocked and the regression is fixed 👍

quietone’s picture

Issue tags: -Needs tests

This issue is tagged for tests, which catch says in #8 are being done in a followup. That followup is #3354211: Tidy up URL generation for asset aggregates. Therefor, I am removing the 'Needs tests 'tag.

Status: Fixed » Closed (fixed)

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