Problem/Motivation

Spun-off from #3370828: Ensure that edge caches are busted on deployments for css/js aggregates.

Library aggregate hashes are based on the library definition (i.e. the YAML) not the file contents, this makes the hashes cheaper to calculate. However if the file contents change without the library definition changing, then the file itself won't change.

Core libraries tend to use VERSION for the library version, this means that many aggregates will be invalidated when a site updates to a new patch release.

Custom libraries (i.e. from custom themes and modules) often don't set any version at all.

Steps to reproduce

Proposed resolution

Add the deployment_identifier to aggregate hashes.
For sites that don't set a deployment identifier, this will be set to Drupal::VERSION - so aggregates that already include \Drupal::VERSION will be unaffected.

For aggregates that don't include it (i.e. only including custom libraries, or only including properly-versioned libraries like jQuery etc.), they'll get invalidated more often.

This brings up an issue though - if a site fully-embraces the new aggregation algorithm, they could set an explicit version on every library, and that will maximise the cache lifetime of each aggregate. If we always set deployment_identifier on hashes, then that strategy won't work the same.

So there are maybe four ways to do this:

1. Always add deployment_identifer to aggregate hashes.
2. Add an extra setting (defaulting to TRUE), and when this is set, add deployment_identifier to hashes, allowing sites to disable it.
3. Instead of adding the deployment_identifer to hashes directly, backfill version in library definitions when it's not set, and then hash based on that. This would mean that any aggregate where all the libraries are versioned would be unaffected. We can do this when building the hash only, so it's transparent to other code too.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#8 3371822-8-add-the-deploymentidentifier.patch1.36 KBscott_earnest
#6 3371822-nr-bot.txt1.05 KBneeds-review-queue-bot
#5 3371822.patch1.5 KBcatch

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes

I thought of some drawbacks when writing this up, but also a possibly solution in #3. So unless there's a fatal flaw in #3 I think we should do that.

chi’s picture

chi commented

Is deployment_identifier changing if a site is reinstalled from configuration?

catch’s picture

catch
he/him
Primary language English
commented

@Chi no, it's based on whatever is set in settings.php or \Drupal::VERSION if nothing is set.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 3371822.patch1.5 KB

Should look something like this.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3371822-nr-bot.txt1.05 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch 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.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Closed (duplicate)

I don't think this is necessary with #3301573: Remove the aggregate stale file threshold and state entry so marking duplicate.

scott_earnest’s picture

scott_earnest commented
StatusFileSize
new 3371822-8-add-the-deploymentidentifier.patch1.36 KB

In our case, we wanted to use the deployment_identifier a little more aggressively, and ALWAYS use it (if set) on every asset. We were running into issues on the build where assets (css etc) were changed in a library whose version was NOT changed, causing the aggregated filename to not change - and would be cached on a CDN without the update to the style. We wanted all assets to be invalidated on every build, and controlled by the deployment_identifier which would change on each build.

The attached patch is another version of @catch 's patch (thank you) from #5. Instead of only using the deployment_identifer when the version is unset, this will use it on every asset. The fallback is the hash of the file contents (the base file has changed: AssetGroupSetHashTrait.php since other patch). Using a deployment_identifer should also decrease the expense of the build, since the system will never need to hash the contents of a file for the version.

PATCH TESTING

Setup
- Core aggregation (css an js) turned on
- Ensure caching is enabled
- Enable a module that has a "libraries.yml" file that includes an asset like CSS
- Set library version numeric e.g. "10.3.6", OR constant "VERSION"
- Look in the source for aggregated file(s), similar to:

    <link rel="stylesheet" media="all" href="/sites/default/files/css/css_hnAzTWJ78jrYDVrHTP6y05aGIS9LmOJ9kqQNNmNPny0.css?delta=0&amp;language=en&amp;theme=olivero&amp;include=eJxdjEEOAyEMAz-0aJ-EArg0aiBVgkD8vj1RqTd7RrZvH2h3IselwhOmdxVNJMHHFu714LcuGEpIOyTR_DriAZRTsraGPvxqKExxcoHGxtm0aSG5fzEKJyNj-Bl3mlxpsPbgyNoL2T7SQZafYX0f_1knM10f_05QDA" />
    <link rel="stylesheet" media="all" href="/sites/default/files/css/css_a1-EIU3hM01tk-bHNLV5AeTBWpvOtvaolQAlOLBNeD4.css?delta=1&amp;language=en&amp;theme=olivero&amp;include=eJxdjEEOAyEMAz-0aJ-EArg0aiBVgkD8vj1RqTd7RrZvH2h3IselwhOmdxVNJMHHFu714LcuGEpIOyTR_DriAZRTsraGPvxqKExxcoHGxtm0aSG5fzEKJyNj-Bl3mlxpsPbgyNoL2T7SQZafYX0f_1knM10f_05QDA" />

- Note these file names, and when they change during the test

Without Patch
- Update library asset e.g. CSS, clear cache
- Aggregated filename does not change

With Patch
- Update deployment_identifier, clear cache
- Aggregated filename changes

Local Development
In a "settings.local.php" file, on a site that uses a development_identifier, and locally you are wanting to have caching turned ON, you can regenerate the aggreated file one of two ways:

  if (!empty($settings['deployment_identifier'])) {
    // Use one of these two methods:
    // Method 1: Unsetting will simply use default asset version invalidation.
    unset($settings['deployment_identifier']);
    // Method 2: Trigger a new aggregated filename on every cache clear.
    $settings['deployment_identifier'] = mt_rand();
  }

PATCH FILE:
- 3371822-8-add-the-deploymentidentifier.patch