Ensure that edge caches are busted on deployments for css/js aggregates

Another option would be to use the deployment_identifier in the asset URL, but I suspect only a small percentage of Drupal sites would have configured this correctly.

Putting the deployment identifier in the asset URL seems fine, we could encode it as part of the hash - all the other logic would work the same such as the handling of valid-but-stale asset URLs.

A lot of sites won't have the deployment identifier configured, but a lot also won't do a release more frequently than core patch releases, which is the default identifier anyway.

Just for some reference -

We have many developers (some frontend only, not aware of Drupal) working on many projects who compile and save CSS frequently while working on tickets. Requiring them to constantly bump the version number (per defined library) to see changes creates a lot of friction. Some themes have several libraries, some have libraries defined dynamically. This is just local development and has nothing to do with deploying to remote servers.

Disabling aggregation is not really a great option because this can significantly impact performance for developers under Docker solutions on Windows or Mac. Especially if they use contributed modules or solutions (a la Acquia Site Studio) that add several dozen CSS and JS files to every page request. At the same time, only having aggregation enabled in production creates an opportunity to miss bugs until they reach production. I understand this change is for real world performance, but does introduce a good deal of friction for local DX.

We typically run everything as prod does, clear cache locally as needed to see changes, commit and deploy from CI. Now it sounds like we'd have to have a versioning scheme, someone assigned to increment versions, update deployment scripts to ... well, you know, all that.

One option is to, yes, bring back the removal of aggregated assets on each cache rebuild. Could this be an opt-in performance configuration option?

Clearing the cache shouldn't matter on local sites though. Its only when changes have occurred from the developer, it isn't constant. It is slower when it is disabled IMO particularly for larger sites.

One more option. If a library omits version, calculate it dynamically based on contents of CSS and JS files it provides.

That would be good.

One more option. If a library omits version, calculate it dynamically based on contents of CSS and JS files it provides.

That would slow things down on production, it might be something for a developer mode though.

some random hash to assets URL

Maybe better to add timestamp instead of random? it will simplify debug and more predictable

If we want the URLs to always change on cache clear, then the css_js_query string is probably the best choice - we've already got it available.

The downside of this though, is that if the contents actually are identical, then CDNs and people's browsers all have to download a new copy of the file for essentially no reason - so again should maybe be a developer mode?

This seems like a pretty big DX change. I know that multiple people ran into this during Drupal 10.1.x development cycle and it looks like more people are hitting this now that they are upgrading site. Wondering if it would make sense to add css_js_query to the hash for now until we have a better solution for development and deployments?

I don't think it's a great trade-off to be transferring potentially multiple terabytes more data across all Drupal sites per year compared to developers having to do a bit of extra work when they change files (or just disable aggregation), but here's what it looks like. If we can put the same logic behind something like #3278493: Make it easier for theme builders to enable Twig debugging and disable render cache that would be better.

I agree that it isn't a great trade-off. Developers potentially turning off aggregation because they are running into problems is not ideal either. 🤷‍♂️

I would love if we could just add the option because that shouldn't be too much work. However, we probably couldn't add that in a patch release unless we can figure out a solution that could be shipped in a patch release.

Discussed with @lauriii in slack - we both think adding to $settings would be fine in a patch release. We can promote that to something like #3278493: Make it easier for theme builders to enable Twig debugging and disable render cache with a UI in a minor release.

Moved to example.settings.local.php per suggestion from @lauriii along with better wording also from @lauriii.

With the scaffold file changes this time.

I am not sure if this would be a concern for anyone, but this change/setting could have a heavy impact on sites which use whole page caching on their CDNs. Basically, if the JS or CSS aggregated file changes, all cached pages need to be flushed from cache in order to reference the new files. This is something that I, were I the tech lead of the site, would prefer to manage with change-sets (ie, bumping version numbers when necessary) rather than having to flush all my CDN cache every time cache is rebuilt, whether the change is worth it or not.

The patch does allow for this to be a setting, so we aren't forcing anyone to do this, but did want to highlight this perspective into this conversation.

@apotek for me this is a developer setting, not for production at all.

The change seems pretty straightforward and seems to do what it claims to on the box.

Just wondering if this needs a test? Looks like we, in general are not very keen on tests on setting in the various *.settings.php-files and/or they are soo old they were created before we test the *BLEEP* out of everything.

Also I think this needs an IS update for the chosen direction and probably also a change record.

Drupal's CSS and JavaScript aggregation is regenerated based on changes to library definitions, not on the files themselves. This means that if files are changed without the library version being incremented, it usually takes some time before the aggregate files themselves are updated.

I really think this needs to be explained more clearly in a CR, I imagine this is going to bite a lot of people.

Even with the setting added in this issue, I'm still a bit confused what actual code deployments are going to look like with CSS/JS changes? In the past, we very rarely had to touch library versions. Now do we need to bump them every time a file in them changes? What happens to old aggregate files?

The patch currently only solves the problem for development. At the moment, aggregation needs to be turned off or developers need to increment version on library definitions every time they introduce changes. Debugging aggregation related issues has been pretty frustrating before this.

For the deployment use case, by far the simplest way to ensure that libraries are updated would be to manually increment the version on library each library definition that introduces changes. This way aggregate files will be regenerated for files that are changed on deployment.

There could be ways to automate this but there's definitely some complexity there if you want to avoid regenerating unchanged aggregate files. This could be achieved by providing a custom constant to be used as version in library definitions, e.g. CUSTOM_CODE_VERSION which a module would then replace in hook_library_info_alter(). The value could be generated based on contents of CSS and JS, based on a deployment id, or combination of the two, depending on the requirements of an individual project.

Core already uses similar pattern with the VERSION constant that we use in library definitions for the version. This gets replaced with Drupal Core version in \Drupal\Core\Asset\LibraryDiscoveryParser::buildByExtension.

At least for any front-end development it definitely makes sense to disable aggregation locally. That's what example.settings.local.php does and continues to do after this. 👍

I'm wondering if we should have a new change record for this aspect of #1014086: Stampedes and cold cache performance issues with css/js aggregation that describes the different strategies for deployment. We probably should include this in our documentation since this also impacts new sites. The most relevant existing documentation page I could find was https://www.drupal.org/docs/updating-drupal/drupal-updates-and-deployments but ideally this would be under https://www.drupal.org/docs/administering-a-drupal-site since this isn't specific to updating Drupal.

As help topics are commited to core new topic "development mode" could be added

This performance settings can be improved
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/help/...

And cache clear topic https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/help/...

Yeah, makes sense. Should we open a follow-up for that?

Yes, I think we should add/update topics to keep inline with each new feature. Yes we using follow-ups like #3358585: Create or update help topics that cover CKEditor 5's module overview text in hook_help()

Even with the setting added in this issue, I'm still a bit confused what actual code deployments are going to look like with CSS/JS changes? In the past, we very rarely had to touch library versions. Now do we need to bump them every time a file in them changes?

Every core release increments VERSION which is used in most library definitions, so deployments including a patch release update will get new aggregates every time.

We could go a step further (in a separate issue) and add $settings['deployment_identifier'] to the hash, that would allow sites that are doing frequent deployments to increment this faster than when they update core and it'd force new aggregates every time. I think this is worth doing and for most sites it would have the same trade-offs as \Drupal::VERSION just provide more options.

What happens to old aggregate files?

They get cleaned up drupal_flush_all_caches() once they're older than system.performance.stale_file_threshold - this logic hasn't changed at all, the only difference in 10.1.x with old files is that the controller will serve the contents of a stale file it can recreate from query strings when the hash doesn't match (without writing it to disk). So if the stale file threshold was set to 24 hours and you had HTML cached in CDNs, those pages will still get CSS and JavaScript aggregates just with reduced performance instead of 404s.

@lauriii @catch, thanks very much for the detailed replies, that clears things up!

+1 for adding the deployment_identifier as part of the hash in a follow up, sounds like a great way to give control while keeping good defaults.

That sounds like a fine solution. I have already adjusted our CI to replace the deployment identifier value with a build number + date.

Along with that, this seems like a good opportunity to show it on the status report too, so this is not needed?

https://www.drupal.org/project/dis

I've opened #3371822: Add the deployment_identifier to CSS and JavaScript aggregate hashes, doesn't really matter what the order is between the two.

Couldn't immediately find an issue to add the deployment_identifier to the status report, but that seems like an obvious thing to add (in its own issue though).

If we go for option #3 in #3371822: Add the deployment_identifier to CSS and JavaScript aggregate hashes, then this setting could control whether it uses the deployment identifier or the css_js_query string, it'd mean only one set of logic for both use-cases then. If that seems like a good option, it might be worth doing them together.

#38 won't work - even if you set versions for everything you wouldn't want to increment for development, but I thought of a possibly better option here.

The reason that cache clears don't clear aggregates now is because of state_file_threshold. stale_file_threshold exists so that pages cached in browsers, CDNs, search engines etc. will continue to have scripts and styles for a while after deployments, defaulting to 30 days. This was added in #721400: Order JS files according to weight, don't change filenames for aggregated JS/CSS.

However, the new asset aggregation system has another mechanism for this - serving a 200 from PHP with no-cache headers for requests where the libraries make sense but the hash doesn't match. So, what if we just remove stale_file_threshold altogether and delete all the aggregate files on cache clear? That would simplify things rather than adding another layer on top.

How does #39 impact performance? Keeping the files in disk is usually low cost so serving them via Drupal is definitely more expensive. Wouldn't this mean that we are prioritizing DX over performance? Maybe that's fine, at least if the cost is not too high.

I already opened #3301573: Remove the aggregate stale file threshold and state entry in August 2022, let's just do that one.

@lauriii it's more expensive but the number of requests for the old files should be very minimal, it's only from stale HTML where the browser or CDN doesn't also have the old files cached too.

I tested #3301573: Remove the aggregate stale file threshold and state entry and, since it just removed the files, without changing the filenames, the old versions are still getting stuck in edge caches. I echo the frustration in #6.

Is this a "wont' fix"? Should frontend developers constantly increment library versions to bust caches when they deploy into an environment with aggregation switched on?

In my hosting settings i have this that came from Platform.sh from a template which has css|js caching for 2 weeks. I predict there are more than a few developers out there effected by this default configuration in their Platform.sh setting, combined with the recent changes in core (although to be honest i don't know why this has never been an issue before, I always assumed a new filename has was created if the contents changed).

    "/sites/default/files":
      allow: true
      expires: 5m
      passthru: "/index.php"
      root: "web/sites/default/files"
      scripts: false
      rules:
        # Provide a longer TTL (2 weeks) for aggregated CSS and JS files.
        "^/sites/default/files/(css|js)":
           expires: 2w

So, fwiw, I removed the 2w setting the changes flow better (I added this ticket to the Platform.sh template). Whether "5m" is a good value or not I'll reserve judgement, but I don't need a different setting for that directory within sites/default/files.

Now I feel like when we do deployments we expect our tools to allow us to cache bust immediately. I want to make sure that I cache bust everything on a cache clear - currently this is not true right? Hence not a duplicate.

As an example of library versions being problematic: If I use Tailwind, and add a class to twig, this causes my CSS to build differently, as tailwind scans my twig for classes. So there's no magic version number of my library that I can set to force the hash to change.

I think it's reasonable to ask for a way to add some timestamp or identifier into the hash generation, to result in different file names, to support a wider variety of deployment processes. That would be either this ticket or #3371822: Add the deployment_identifier to CSS and JavaScript aggregate hashes (also marked as a duplicate).

@sime yeah I think we still need something here, might as well use this issue but I've retitled for the main remaining thing.

I think there are three approaches we can take:

1. Add the deployment_identifier into the hash.
2. Add the css_js_query_string into the hash
3. Add the css_js_query_string into the query string (i.e. what we do for individual files) without changing the actual filename.

All of 1-3 are about the same to implement, it's mostly a case of deciding what's best.

There's also #4 which is a bit more complex but also more targeted:

When a library doesn't specify a version, file_get_contents() the files, hash the raw contents and use that in place of version. We'd need to do that in AssetGroupSetHashTrait so it would run both when building the main page, and also when files are generated. This would have a performance cost when building the filenames, although still a lot less than the pre-Drupal 10.1 logic, and better cache hit rates when things haven't changed.

Love the way you just change the title of this ticket to something completely different!

FYI, after running into problems with aggregated css and js after upgrading to 10.1 I have decided the easiest thing for now is to disable css and js aggregation.

As these changes have not been documented in a clear and concise manner, I am a little perplexed as to what the best practice or intention is here? Is it to disable to css and js aggregation in Drupal 10.1?

@2dareis2do, issue titles change all the time.

#3301573: Remove the aggregate stale file threshold and state entry already forces the aggregate files to change on disk when caches are cleared, which was the first suggested solution in this issue. I had opened that issue in August 2022, nearly got it committed, but it got held up, as unfortunately many issues do in core. If more people helped review issues and get them ready for commit, then we might have quicker progress, but instead a lot of people make passive aggressive comments instead.

For local development the problem is solved in 10.1.1 which was released yesterday, the worst is that you might need to shift-refresh the page you're working on. Have you updated to 10.1.1?

We've still got the case of edge-caching when versions aren't specified for a library - since the original issue report here covered both the deployment and local development cases, it's fine to re-purpose this issue for the remaining bits.

--

I've updated the issue summary, overall I think my preference is probably "Add css_js_query_string to the asset URL query string" - it will be easy to implement and it has the least bad side-effects, but with "Use asset hash as version when no version is specified" as a close second.

Yes, we already use that practice for non-aggregated URL's, where we either add the library version or a hash, so I think applying the same pattern to aggregated links makes sense and shouldn't break anything that worked before?

There are cases where you ignore the query string or at least parts of it, like dynamic page cache does, query strings often have things like tracking/utm stuff which doesn't need to be cached differently, but for asset files, per above, I agree that it would be very uncommon to do that and could have caused problems already before.

Thanks @catch

I do appreciate your efforts on this, but at the same time it is frustrating when things break. Apologies if this comes across as passive aggressive comments.

I also appreciate that tickets titles do change but I am not a fan of this as it appears to be constantly moving the goal posts to suit. Perhaps a better approach is to open up another ticket for the edge case thing.

Updating the case summary now. Sounds like rewriting history. Great!

Will give 10.1.1 a go and see if that helps.

I also appreciate that tickets titles do change but I am not a fan of this as it appears to be constantly moving the goal posts to suit. Perhaps a better approach is to open up another ticket for the edge case thing.

As far as we know there's no active bug left apart from edge/browser caches, we could open a brand new ticket but that would mean closing this as duplicate again.

Here's a patch for the query string, it's definitely the easiest solution and has few drawbacks.

When a library doesn't specify a version, file_get_contents() the files, hash the raw contents and use that in place of version. We'd need to do that in AssetGroupSetHashTrait so it would run both when building the main page, and also when files are generated.

This approach is most attractive to me as a front-end developer. Perhaps instead of no version specified (opt-in by implication), this could be explicitly opt-in via keyword (à la VERSION). Maybe HASH, FILEHASH, or similar?

Tried updating to 10.1.1 but have same problems on prod. i.e. no css or js with css and js aggregation enabled.

Website is running Apache with nginx reverse proxy. Also has Smart static files processing enabled (running on
typical Plesk Obsidian setup). Have tried disabling Smart static files processing.

Have tried all variations with the same result, e.g. apache, nginx, FastCGI, FPM, Dedicated FPM

CSS and JS aggregation enabled and working locally running on apache server (was getting no aggregation before switching from nginx setup).

On prod server I get `Failed to load resource, server responding status of 500' for referenced aggregated css and js.

Interestingly I can see that web/core/misc/touchevents-test.js does seem to get aggregated and served.

Test failure was random so moving back to needs review.

@2dareis2do

On prod server I get `Failed to load resource, server responding status of 500' for referenced aggregated css and js.

That's a different from the issue reported here. This issue is about whether the filenames change or not when files update, not aggregation itself failing. You should check your server logs to see what's causing the 500 error. Also check whether you need to make any nginx changes per https://www.drupal.org/node/2888767 (although that will depend on your existing nginx configuration).

@catch,

The only errors I am seeing on the server log are

Error: Class "Drupal\bootstrap\Bootstrap" not found in include_once() (line 31 of /var/www/vhosts/mydomain.com/httpdocs/web/themes/contrib/bootstrap/bootstrap.theme).

2023-07-07 17:44:49 Error 81.99.202.74 AH01071: Got error 'PHP message: Uncaught PHP Exception Error: "Class "Drupal\\bootstrap\\Bootstrap" not found" at /var/www/vhosts/mydomain.com/httpdocs/web/themes/contrib/bootstrap/bootstrap.theme line 31', referer: https://mydomain.com/ Apache error

When disabling aggregation I do not get this error?

Also, as per https://www.drupal.org/node/2888767, I have never had to modify the nginx conf previously. e.g.

location ~ ^/sites/.*/files/(css|js|styles)/ {
try_files $uri @rewrite;
}

Adding this value does not seem to help.

Not sure why status has changed?

Not sure why status has changed?

Sometimes things happen. Setting back to NR.

Patch in #50 works for me. When I have aggregation on, and I clear the cache (simulating a deployment), I see a the new deployment identifier ont the end of the aggregrated file urls. This busts cache in my browser.

Screenshot of before/after diffing the source of the page.

@2dareis2do please open a new issue for your bug report, it's unrelated to this issue, and looks like it might be specific to the bootstrap theme which is a contrib module. If you can get a backtrace for that error, it might be possible to narrow down.

For the follow up requested in #60

Also for the IS update and change record tags

Both tags were added before the issue was repurposed. The IS was updated in #46. I don't think these changes would warrant a CR? Happy to be wrong though.

Yeah this is a very small behaviour change that wouldn't need a change record.

If we did "Set library versions base on calculated asset hash when the version is empty" that might be worth a CR since there's a way to stop that behaviour (adding a version), I still don't have a very strong preference between the two, although the query string is definitely the easiest.

Here is "Set library versions base on calculated asset hash when the version is empty". Actually doesn't add much complexity because everything we need is in the asset array already.

Thanks @catch

I think you are right. Tried switching to Oliveiro and the issue seems to go away.

Looks similar to https://www.drupal.org/project/bootstrap/issues/3369979

Thank you for the patches catch!

At my workplace, we typically dev & deploy sites similar to what sime posted in #43, with aggregated assets cacheable by exact URL for a long time. I would be very happy to see #64 go in to core - and perhaps that is the best option, since it should also fix any regressions that any folks who don't define a version number are experiencing with 10.1.

Personally I also like the idea of a setting to opt in to including the deployment_identifier in hashes, since we usually do have one set. But I imagine not everyone does.

Added separate issue for broken css and js aggregation when running on nginx

https://www.drupal.org/project/drupal/issues/3373929

Adding test coverage. Test only patch is also the interdiff.

Unused use statement..

via @lendude in bugsmash - some copypasta from logic copied from a helper message, we don't want to early return.

Asked in slack and this seems like a major issue.

I'm relying on #69 fail/pass

The change seems small and comments help.

Based on previous comments too seems to have solved the problem for others.

Going to mark.

Couple of questions, nice work, love the use of xxhash 🏎️

1. +++ b/core/lib/Drupal/Core/Asset/AssetGroupSetHashTrait.php
   @@ -36,6 +36,12 @@ protected function generateHash(array $group): string {
   +      if ($asset['version'] === -1) {
   

   Should we add a constant for -1 that reflects its special status?

   I think we need to document this in the docblock of \Drupal\Core\Asset\LibraryDiscoveryParser::parseLibraryInfo

2. +++ b/core/lib/Drupal/Core/Asset/AssetGroupSetHashTrait.php
   @@ -36,6 +36,12 @@ protected function generateHash(array $group): string {
   +        $normalized['asset_group']['items'][$key]['version'] = hash('xxh64', file_get_contents($asset['data']));
   

   Nice, is this our first use of xxhash ?

3. +++ b/core/tests/Drupal/FunctionalTests/Asset/UnversionedAssetTest.php
   @@ -0,0 +1,95 @@
   +    $this->rebuildAll();
   ...
   +    $this->rebuildAll();
   

   Can we document why these are needed with comments?

   We're trying to solve a caching bug here, so needing a rebuild all in the test feels iffy - can we be sure its not masking a future issue?

   Can we be more granular with this, e.g. just invalidate the library info cache or similar?

RTBC + 1 we were encountering the same issue, broken css and js after a new deploy and it was random, sometimes fine, sometimes not. A hard browser refresh would work but as we know users are not going to do that. However as soon as you perform a cache clear the issue comes back. With the patch from #71 the issue has resolved itself for us. Plus this only occurs since 10.1.

Removed

Fixing #74:

#1. discussed with @larowlan and I'll open a follow-up for it, the -1 is pre-existing and the place it's added has an inline @todo from 2014 to use module version there instead (which would still need a fallback for unversioned modules, so I guess if we ever resolve that @todo the logic here would survive), but pre-existing issue.

#2. Yes! But we do have #3307718: Implement xxHash for non-cryptographic use-cases to retrofit it everywhere else.

#3. Very good point, we can drop the first one, and only need to clear two bins for the second. Added a comment about what's going on there.

@glynster the bug you described is more likely to be #3371358: When AssetControllerBase delivers existing file should add content-type.

@Chi: maybe but we should also do that in AssetOptimizationTest where this logic was copy/pasted from, maybe another follow-up? I'd like to get this in before the next 10.1 patch release if possible. I guess we could also adjust both tests here though.

As you said @catch #3371358: When AssetControllerBase delivers existing file should add content-type resolved the problem right away.

Points from #73 appear to be addressed

@catch can you link the follow up whenever you get a chance

Thanks!

Opened #3376263: Tighten xpath selectors to decrease complexity in tests for the excellent suggestion by @Chi in #75.

Opened #3376273: Add a constant for the -1 version in LibraryDiscoveryParser. If we resolve the 2014 @todo to add module or theme version when it's available, this would reduce the amount of file_get_contents() we do here (while still ensuring that caches are busted when new versions of modules or themes are deployed), which would be good. We'll still need the fallback here for custom modules/themes with no version, which is the main use case too.

The suggested fix looks like the simplest way to solve this, but locally #77 passes with or without the fix, trying that on testbot too.

#83 should have failed.

Oh no, #69 failed so it'll be cache clearing changes in #77 probably. I'll take a look again.

OK this should do it.

The first cache clear is necessary to make the test fail in HEAD, because without it we're not looking at the very latest asset URLs that we should be, so it sees a change due to the later cache clear even though it's for a different reason. So added a more minimal clear back.

Test failed + passed as it should. 👍

Committed and pushed 3041dbfe26 to 11.x and a6460005b0 to 10.1.x. Thanks!

So, now that the patch has been added, does that mean we can get around the issue by adding

"version: -1" in the .libraries.yml file?

For example:

base:
  version: -1
  css:
    theme:
      dist/css/tipi.bundle.css: {}
      dist/css/tipi.print.css: {}
  dependencies:
    - core/drupal

and then it will re-aggreg on cache clear?

It looks like "-1" is the default version set when a library does not declare a version:

https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/lib/Drupal/...

Ah! That makes more sense.

So, I should just remove the version line all together.
Is this workaround documented anywhere, apart from here? It's pretty vital information :)

Just to confirm: Is the way to make sure that Drupal always regenerates aggregated assets on cache clear without having to bump the version number on each library that has changed to remove version altogether from our library declarations in *.libraries.yml files?

For anyone looking for a definitive answer to this as I was: I received confirmation in Drupal Slack that removing the version key from libraries, regardless of where they are defined (core/contrib/custom), is the correct way to ensure that edge caches are busted on deployments for aggregated CSS/JS. I agree with @ras-ben in #95 that this should be documented somewhere and even warrants a change record.

I believe we need a change record for this

Does this need a change record or change record updates? Which is it? It’s tagged with both.

The version: -1 needs CR and there's follow-up which probably blocked it #3376273: Add a constant for the -1 version in LibraryDiscoveryParser

not sure update for https://www.drupal.org/node/2888767 is needed

There’s an argument to be made that CR 3301716 should be updated to include information about how to opt out of the new behavior (namely: by removing version keys from any library definitions that should be re-aggregated on cache clear/site deploy).

Made this change to the CR. Anyone should feel free to update change records to add additional useful information.

https://www.drupal.org/node/3301716/revisions/view/12744806/13230795

Back to fixed. The -1 version is an existing internal thing and doesn't need a CR.

I don't understand how this commit helps with the deployment process.

We have a webfactory which handles over 20 components, that means +20 drupal libraries.

Our developers apply changes to those libraries quite often, and we deliver those modifications to production once a week.

Does this solution mean that we have to remove all "version" keys of all our libraries to ensure that any change will be reflected in production ?

I would rather have implemented the solution with "2. Add the css_js_query_string into the hash" as in #21

The alternative is to update the version key on any libraries that you know for sure changed in a given release. That is likely too onerous a requirement in your case, so the best option is to remove the key entirely. When Drupal encounters a library without a version key, it will hash the contents of each file to use as the file’s “version”. It is my understanding that this will result in only the files that have actually changed getting cache busted.

@yonailo Can you share details on why removing version is impractical, unfeasible, or otherwise does not fix the problem in your scenario?

Well … if this is the new way of working …we can remove all our version keys, it is definitely feasible … but we used to use the versions keys to track the version of the library (specially when it is external)…

We were using the contrib module advagg under D9 and we didn’t have to proceed like this… I have been a bit surprised when discovering this issue…

I’ve always thought that the version key was merely informative but I was wrong. I understand that the way Drupal10 works now is correct and coherent so we will try to adapt to it.

Moreover, we can continue using versions… we just need to be careful and not forget incrementing them when we make a modification.

Thank you all for your support and all your explanations in this thread ! ^^

Using version is certainly an option. I didn’t mean to dissuade you from doing that if it works for your team and the specifics of your project and workflow. In our case, we found that it was A) too easy to miss during development, and B) too difficult to catch during review. This was especially problematic for us because our PR preview environments did not exhibit the kind of behavior you get from production when a library is updated but the version key is not (since the PR preview is essentially a fresh build with no old assets in the cache). So after a handful of deployments to production that served stale assets (requiring quick follow-ups to bump the relevant version numbers), we decided on going with the safer option and just remove version altogether.

We have also applied the patch #21, as we want to regenerate all hashes with drush when doing a production deployment.

Minor edits to issue summary

Precisely which libraries.yml files is one required to check, and how, if at all, can this be further diagnosed?

I have a 10.2.1 site where aggregation is still broken despite removing the 'version' line from the only two files (excluding core.libraries.yml obviously) I can find in the codebase: those of my custom public and admin themes.

At the moment there is a lack of good end-use documentation and a growing number of confused people elsewhere who have given up and just disabled it.

At the moment there is a lack of good end-use documentation and a growing number of confused people elsewhere who have given up and just disabled it.

I would totally agree with that.

Having the issue on Drupal 10.2.2.
Please explain what should be done to get aggregation working.

You can try to validate the server configuration meets the requirements for js/css aggregation files.
Please check #3384272: Allow to validate Apache/Nginx configuration requirement for aggregation folder before enabling aggregation.

For all of you who, like me, first landed here in search of a solution and somehow ended up there: https://www.drupal.org/project/redirect/issues/3373123. If I deactivate the 'Enforce clean and canonical URLs' option of the redirect module on one of our multilingual pages (only for debugging - we currently prefer to work with non-aggregated CSS files instead of disabling the "Enforce clean urls" option), the aggregated CSS files are loaded correctly. All suggested solutions in this issue did not work. The patch #39 in the linked redirect-issue applies, but does not work in our case either. However, the cause is the same. Perhaps this is a hint for all those for whom the CSS aggregation also no longer works. Checking the server configuration with an updated patch from https://www.drupal.org/project/drupal/issues/3384272 shows that everything is ok/correctly configured.
* Server: Apache
* Drupal 10.2.2
* redirect 1.9 (patched)
* multilingual

If I deactivate the 'Enforce clean and canonical URLs' option of the redirect module on one of our multilingual pages, the aggregated CSS files are loaded correctly.

It didn't work for me. We are losing search engine position because of excessive loading of JavaScript and CSS.

Yep, for what it's worth, the Redirect config change didn't work for me either (and these are not multilingual sites).

I noticed this fix was not included in 10.2.4 (according to the release notes) but is marked "Closed (fixed)". I'm curious when it will be included in a release, because I am still running into this issue even after updating to 10.2.4.

@caspervoogt the code changes for this issue are included in 10.2.4, it was committed before 10.2.0 (in 10.1.x)

I would suggest opening a new issue describing the issues you're facing and we can continue investigating there.

@acbramley thanks. I think my issues are probably environment/server-related

Since we have been also burnt by this, I have also updated the related the "Adding assets (CSS, JS) to a Drupal module via *.libraries.yml" documentation page to follow best practices and warn about incorrect usage of "version" in library definitions Since Drupal 10.1.2.

https://www.drupal.org/node/2274843/revisions/view/13464318/13535225

For us the issue was Nginx.

Updated the Vhost from:

location ~ ^/sites/.*/files/styles/

to

location ~ ^/sites/.*/files/(css|js|styles)/ {

Resolved the issue right away.