Problem/Motivation

When building asset aggregate URLs, we use a combination of library versions for libraries that have them, and file contents for libraries that don't - this is to save hashing every file in an aggregate.

After #3505776: Retain asset aggregates for 45 days by default to reduce generation of identical files it is a bit more awkward if you update a file without updating a version for a library, because drush cr doesn't automatically clear the aggregate files. There are ways to disable the behaviour in local dev etc. but it can be a gotcha.

Thinking about whether to open this issue at all, I suddenly realised that all of core's own libraries are defined with VERSION which changes every patch release, even though the individual files often don't change for months or years.

This means that for core libraries, the logic is doing the opposite of what we want it to - results in more changes to aggregate filenames rather than less.

We can remove VERSION from all core libraries, but given it would also make things easier for contrib/custom themes and modules, I think always using the file contents is probably easier. Can profile it to see what the impact is.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3589208

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review

catch opened merge request !15710

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work 
       ├ Exception: Warning: file_get_contents(themes/my_theme/css/dropbutton.css): Failed to open stream: No such file or directory
       ├ Drupal\Core\Asset\CssCollectionOptimizerLazy->generateHash()() (Line: 45)

       ├ Exception: Warning: file_get_contents(core/modules/system/tests/themes/test_theme/kitten.css): Failed to open stream: No such file or directory
       ├ Drupal\Core\Asset\CssCollectionOptimizerLazy->generateHash()() (Line: 45)

Seems related to the change here?

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Yes very related. Added a file_exists() here because aggregate generation itself also has to cope with missing files already - so even though the failures are to some extent 'invalid' library definitions, I think we want to allow 'stub' library definitions to not break the hash generation logic.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems to have fixed the issue LGTM

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Small performance tweak suggested on the MR.

heddn made their first commit to this issue’s fork.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs work » Needs review

Applied suggestion.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Reviewed & tested by the community

Seems like a reasonable change. I think I can RTBC still since I just applied a suggestion after reviewing if it made sense. It does.

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

I thought the idea was to detect changes to the file by hashing the file contents, so I don't understand whyfile_get_contents was removed. Otherwise, the comment needs to be updated to match the fact that the file contents aren't being loaded, as well as the IS.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs work » Reviewed & tested by the community

https://www.php.net/manual/en/function.hash-file.php hashes the file contents not the file name. No?

godotislate’s picture

godotislate
he/him
commented

Ah, sorry, I missed that it was hash_file.

godotislate closed merge request !15710

  • godotislate committed ef1f9778 on 11.x 
    task: #3589208 Always use the file contents to determine asset aggregate...

  • godotislate committed 61ef4c65 on main 
    task: #3589208 Always use the file contents to determine asset aggregate...

  • godotislate committed 4def42a0 on 11.4.x 
    task: #3589208 Always use the file contents to determine asset aggregate...
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 61ef4c6 to main, ef1f977 to 11.x, and 4def42a to 11.4.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

mstrelan’s picture

mstrelan commented

This is great I could have used this last week on a site that had version defined in the theme libraries. Pantheon global CDN decided not to purge the aggregate URL and I was stuck with the old version after deployment.