CssCollectionGrouper should respect media property from CSS asset definition

Thanks for the bug report and the patch! Looks like a regression from 🐛 Aggregation creates two extra aggregates when it encounters {media: screen} in a library declaration Fixed .

Moving to needs review to trigger the CI.

The idea behind 🐛 Aggregation creates two extra aggregates when it encounters {media: screen} in a library declaration Fixed was to put media = screen into 'all', because for the critical path (serving an HTML page) we don't want the extra http request, I think this patch will break that, or in other words it's by design to an extent.

We might want to reverse the condition and change media = screen to media = all explicitly, then leave everything else (including print) to create a separate aggregate - would that fix things for your use case?

@tikaszvince all three of your libraries in the issue summary have preprocess: false so they shouldn't be aggregated at all in the first place?

I'm a bit confused why the patch above doesn't break the test that was added in 🐛 Aggregation creates two extra aggregates when it encounters {media: screen} in a library declaration Fixed

@catch Yesterday I was in a hurry to fix this on live project as soon as possible. I went forward step-by-step. First I've tried to add preprocess: true, than add my media query as an attribute, than I tried to mark these css files as minified (we compile these files from SCSS as compressed as possible). None of these changes created any link element with the required media query attribute. Currently this is the combination which works for me: the attached patch, and the preprocess: false together produces the required link elements with the required media attributes.

If i revert my patch, and revert my library definition to contains only weight and the media queries (e.g. media: 'all and (min-width: 768px)') none of these CSS files delivered to the client. No separated, no aggregated, no part of an aggregated file.

I couldn't find any explanation yet but sometimes some CSS file is just missing from the aggregated files, sometimes whole libraries skipped.

And one more weird behaviour which i found and i don't know if it is related or not. Before 10.1 when I change something in my CSS than clear the cache the new aggregated CSS created from the new files. This worked without any additional step, no version string or variable change was needed to get the CSS changes to work. After 10.1 I'm this method does not work. I've tried to clear the cache with drush, from the admin menu, full cache rebuild, or step by step (flus CSS/JS cache, theme cache, render cache, etc). I've tried to delete css directory, but the changes does not appeared in the aggregated CSS assets.

So overall i think we have a very severe regression I cannot understand. Why Drupal thinks there is only two type of media (print or all) when the specification says media attribute can be any media type or media query?

@tikaszvince If you read the patch on 🐛 Aggregation creates two extra aggregates when it encounters {media: screen} in a library declaration Fixed , for any non-print stylesheet, it adds it to the 'all' group, but it also inlines the media attribute in the CSS aggregate. So it's not that it thinks they're the same, it's that it thinks they should be put into the same aggregate group to reduce unnecessary HTTP requests.

This is because if you have multiple stylesheets for 'all' and 'screen', it makes no sense to have them in different aggregates - they're going to be used when serving the web page, which is where the HTTP requests matter. If you explicitly do not want your stylesheets aggregated, then preprocess: FALSE is the correct setting.

For the files not updating on a cache clear, see 🐛 Ensure that edge caches are busted on deployments for css/js aggregates Fixed which will go out in 10.1.2, if you're checking the contents in your browser and not seeing the change, then that might be the issue. However it should not be possible for the files to be re-created on-disk without changes, because they're created from the files themselves - if you think that's happening regardless, check the actual contents of the file on the filesystem to be sure.

I'm understand the intention to reduce the number of HTTP queries. Overall i think the current implementation does not allow us to differentiate between assets, or allow fine tuning how we want to group and/or link CSS files.

Currently if i run without my patch, and apply the latest patch from 🐛 Ensure that edge caches are busted on deployments for css/js aggregates Fixed on 10.1.1 and revert my library definition to this:

global-styling:
  css:
    theme:
      assets/css/style-00-base.css:
        weight: 0
      assets/css/style-01-tablet.css:
        media: 'all and (min-width: 768px)'
        weight: 2
      assets/css/style-02-desktop.css:
        media: 'all and (min-width: 992px)'
        weight: 3
      assets/css/style-03-large.css:
        media: 'all and (min-width: 1440px)'
        weight: 4

none of these CSS files or any part of its content delivered to client. Not as a single file or part of any aggregated CSS file.

This defines 4 CSS files (I removed the sourceMap comment from the compiled css files). Two contains additional @media queries the other two does not. These files are not containing any syntax error. One contains a direct import from Google Fonts.

My first thought was it must be some error in the compiled CSS files which the optimizer cannot handle. But if i replace the content of these files one simple rule (body {background: red;}) still skipped, so i think the content of these files are not count in this situation.

And there is nothing in the watchdog which could help me why these files are skipped from the aggregation at al.

@tikaszvince can you double check that the library you're expecting to see in the aggregate is actually being requested when the assets are generated?

For example applying something like the attached patch to core and check the list of libraries in there.

If the library is there, and the aggregates don't contain the contents, then there could be an issue in CssOptimizer::optimizeGroup().

If the library does not appear there, then it could be an issue that either the library just isn't added to the page you're requesting, or a bug in CssCollectionOptimizerLazy::optimize()

I would also try removing the media queries from the library definition just to test if the contents show up in the file.

Overall i think the current implementation does not allow us to differentiate between assets, or allow fine tuning how we want to group and/or link CSS files.

If the intention is just to serve the files separately, then preprocess: false covers that case.

There were multiple changes in the aggregation system in 10.1, and they were quite big changes, and there's been a few bugs found since 10.1 was released, but at the moment all the known regressions are fixed (although some not into a patch release yet), this is the only remaining issue that's been reported.

I've simplified my library definition in the theme.

global-styling:
  css:
    theme:
      assets/css/style-00-base.css:
        weight: 0
      assets/css/style-01-tablet.css:
        weight: 2
      assets/css/style-02-desktop.css:
        weight: 3
      assets/css/style-03-large.css:
        weight: 4

My theme info file contains

libraries:
  - [MY_THEME]/global-styling

When CSS aggregation is disabled via settings.php $config['system.performance']['css']['preprocess'] = FALSE;. All the 4 css file is loaded on the page and the styles are applied on elements.

When I enforce CSS aggregation and load the same page The HTML document contains only 2 link element

https://[...]/sites/default/files/css/css_U1alzjUJOwF2aPk0mkaT8DevuUlYGLTwyS9VaScX9J0.css?delta=0&language=hu&theme=[...]&include=eJyNkFluwzAMRC-kWEcSGJu15XJRKdmpc_oyUYEUrT_6o2U4BOeRYT7ABAppukLFiDvacVvQMNSjNuT4kAPLs5zagoxnEmOtMGMNoxpGUWOgfMf-nWwrQEMxnc19j9beBSt8Bv6dgXVCk3y3l3EmvQJdajsoy_zSFwT3_vBpalr61LVeRtX3jAG31F9-caEMMjrpifh_pye1b7oszeM63_qx-fKGN4cPe8Zbjc9zcJ6N8C8nyp5NhVFaAMkMDadUR1Mip4gqqfhKEylMZ_Xc61-1h7Db

https://[...]/sites/default/files/css/css_pV_APjWugivh9z5_eKDEzV5JFUx6aylPuzZCI6_7kW8.css?delta=1&language=hu&theme=[...]&include=eJyNkFluwzAMRC-kWEcSGJu15XJRKdmpc_oyUYEUrT_6o2U4BOeRYT7ABAppukLFiDvacVvQMNSjNuT4kAPLs5zagoxnEmOtMGMNoxpGUWOgfMf-nWwrQEMxnc19j9beBSt8Bv6dgXVCk3y3l3EmvQJdajsoy_zSFwT3_vBpalr61LVeRtX3jAG31F9-caEMMjrpifh_pye1b7oszeM63_qx-fKGN4cPe8Zbjc9zcJ6N8C8nyp5NhVFaAMkMDadUR1Mip4gqqfhKEylMZ_Xc61-1h7Db

Both redirected with HTTP 302 to self

https://[...]/sites/default/files/css/css_U1alzjUJOwF2aPk0mkaT8DevuUlYGLTwyS9VaScX9J0.css?delta=0&language=hu&theme=[...]&include=eJyNkFluwzAMRC-kWEcSGJu15XJRKdmpc_oyUYEUrT_6o2U4BOeRYT7ABAppukLFiDvacVvQMNSjNuT4kAPLs5zagoxnEmOtMGMNoxpGUWOgfMf-nWwrQEMxnc19j9beBSt8Bv6dgXVCk3y3l3EmvQJdajsoy_zSFwT3_vBpalr61LVeRtX3jAG31F9-caEMMjrpifh_pye1b7oszeM63_qx-fKGN4cPe8Zbjc9zcJ6N8C8nyp5NhVFaAMkMDadUR1Mip4gqqfhKEylMZ_Xc61-1h7Db

https://[...]/sites/default/files/css/css_pV_APjWugivh9z5_eKDEzV5JFUx6aylPuzZCI6_7kW8.css?delta=1&language=hu&theme=[...]&include=eJyNkFluwzAMRC-kWEcSGJu15XJRKdmpc_oyUYEUrT_6o2U4BOeRYT7ABAppukLFiDvacVvQMNSjNuT4kAPLs5zagoxnEmOtMGMNoxpGUWOgfMf-nWwrQEMxnc19j9beBSt8Bv6dgXVCk3y3l3EmvQJdajsoy_zSFwT3_vBpalr61LVeRtX3jAG31F9-caEMMjrpifh_pye1b7oszeM63_qx-fKGN4cPe8Zbjc9zcJ6N8C8nyp5NhVFaAMkMDadUR1Mip4gqqfhKEylMZ_Xc61-1h7Db

The new request to the redirected URL is served with HTTP status 200.

It seems the AssetControllerBase::deliver does not called at all, the include_string.txt does not created. (I make a syntax error on that file and it does not throw an error).

I placed a dump on these places: AssetControllerBase::deliver CssCollectionOptimizerLazy::optimize, CssCollectionOptimizerLazy::optimizeGroup and CssOptimizer::optimize methods to dump the arguments to different files. Patch attached.

Only the CssCollectionOptimizerLazy::optimize created any file, so i think only this method was called. This dump contains all the CSS files defined by the library.

I'm using Apache with these rewrite rules

RewriteEngine on
RewriteBase /
RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-d
RewriteCond %{REQUEST_URI} !=/favicon.ico
RewriteRule ^ index.php [L]

I'm not allowed to attach project specific codes and examples here. I understand that every project has its own constellation and it is possible our code messing up something. What should I check to figure this out?

It seems the AssetControllerBase::deliver does not called at all, the include_string.txt does not created. (I make a syntax error on that file and it does not throw an error).

If this happens, you need to do a full cache clear, and also make sure you've deleted the sites/default/files/css directory (or wherever the directory is - although that second step should be unnecessary with Drupal 10.1.1. If you've done a full cache clear and deleted the files, then the request can actually hit the controller path to generate the file, otherwise by design apache will serve the file from disk.

Also to double check - can you confirm you're using Drupal 10.1.1 (as opposed to 10.0.0 or a beta/rc), and also can you confirm whether you've got the contrib advagg module enabled or not?

I've tried cache clear with varios ways, drush cr, also drush cc drush && drush cc theme-registry && drush cc css-js && drush cc render and as I mentioned before from the admin toolbar, hitting the "Flush all caches" menu item, and also tried the "Clear all caches" button on /admin/config/development/performance page.

In my settings.local.php I'm using null cache backend for render and dynamic page cache. In live environment we use Redis cache backend.

I'm using Drupal core 10.1.1 with your latest patch from 🐛 When AssetControllerBase delivers existing file should add content-type Fixed and with Drush 12.1.2. No advagg module installed.

Installed extensions implementing hook_library_info_build: dropzonejs
Installed extensions implementing hook_library_info_alter: entity_browser_enhanced, locale, gin_toolbar, dropzonejs, field_group, system, image_widget_crop, ckeditor5, responsive_image
Installed extensions implementing hook_css_alter: gin_toolbar, gin

@tikaszvince can you check the actual contents of the css directory as well per #12?

On cache clear (no matter the method) the sites/default/files/css directory is deleted. When i reload the page in the browser, the same files with the same name and content placed.
If i load the same page with curl after a cache clear (to make sure we generate only the HTML output and prevent downloading any of the assets) the CSS directory does not created. If I download one of the CSS assets linked to header the CSS directory and the requested file is appeared.

@tikaszvince

So if the CSS files get recreated, then:

It seems the AssetControllerBase::deliver does not called at all, the include_string.txt does not created. (I make a syntax error on that file and it does not throw an error).

Is this still the case? Or are you getting output now?

Still the same.

This is how my project filesystem looks like after clearing the cache, also the library definition visible on this screenshot.

This is how the page appears in the browser when i load it after a cache clear:

After I see the page only the lazy-optimize.txt file appears and the aggregated CSS files also created.

And this is how the page appears in the browser when I disable CSS aggregation clear the cache and load the page

OK it's not possible for the files to get written without AssetControllerBase::deliver() getting called, unless something is very, very wrong with your install.

Please try the attached patch - make sure you clear caches and shift refresh the page, and also confirm that the aggregate files are not there before you refresh, but are there afterwards.

Sorry uploaded the wrong patch...

I think I found the cause of this strange behaviour. Our CSS contains this line

@import url('https://fonts.googleapis.com/css2?family=Fraunces:opsz,wght@9..144,400;9..144,600&display=swap');

When I remove this line and add the URL to the library as an external asset the CSS aggregation just works fine. So I think this is an other issue 🐛 CssOptimizer silently drop the attached library when a CSS file @include an external CSS. Postponed: needs info .

Is it possible to handle other typed of media than "all" and "print"? Is it possible to create a separated aggregated file if a library defined a media query with the asset definition?

lf you just want one, separate, file, you should add preprocess: false to the library definition, which is what you're already doing.

I think what you are asking for though is that if you have multiple files with media: 'all and (min-width: 992px)' that these would be aggregated together into the same group, which is old behaviour. However, do you actually have multiple files with the same specific media query like that, or is it just one each? I can't think of a situation where there'd be multiple except for a custom theme which can control exactly how many stylesheets it has. And if there are different files that show up on different pages with the same media query, then having those as separate files is not a bad idea anyway - because it saves downloading the same stylesheet content twice. Given that, moving this specific issue to a support request - I think the current behaviour is correct after 🐛 Aggregation creates two extra aggregates when it encounters {media: screen} in a library declaration Fixed and that the API allows you to do what you want to do. It's also producing correct CSS apart from the @import issue.

Ok, thank you for the help!

Since we found the root cause of my original problem and we have a solution for it i think this is not a bug anymore.