Problem/Motivation

The libraries defined in libraries.yml use the core shortcut "version: VERSION". This causes the library manager to substitute the core drupal version. This is correct for core modules, but not for contrib modules. These version numbers should be replaced with actual version numbers, which are bumped when the version increases. Failure to do so may cause stale libraries to be loaded in production for a long time. See https://chromatichq.com/insights/drupal-libraries-version/ for a discussion.

Note: Using the commonly-seen "version: 1.x" will resulting even worse caching because it won't be invalidated by a core version update.

Steps to reproduce

1. Load a page with caching disabled (local dev).
2. Make a change javascript or css in a library.
3. Refresh the page in the browser. Notice that the change wasn't seen.

Proposed resolution

Supply an initial version for all libraries (e.g. 1.0.1), except those that depend only upon core resources. Then establish a procedure for updating these in any commits modifying dependencies of the particular library. Once option would be to use the version of the next upcoming version of webform (e.g. 6.2.4) whenever a dependency changes. As far as I know, these version numbers are used only for caching so any unique version number is okay.

Issue fork webform-3460220

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

DanChadwick created an issue. See original summary.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

+1 on this

For a start, I would just remove the "version" declaration which would lead to file content hashing in \Drupal\Core\Asset\AssetGroupSetHashTrait::generateHash() because that would default back to -1.

       // If the version is set to -1, this means there is no version in the
      // library definition. To ensure unique hashes when unversioned files
      // change, replace the version with a hash of the file contents.
      if ($asset['version'] === -1) {
        $normalized['asset_group']['items'][$key]['version'] = hash('xxh64', file_get_contents($asset['data']));
      }
mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

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

mxr576 opened merge request !491

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Status: Active » Needs review
steven jones’s picture

steven jones commented

@mxr576 Thanks for this.

I'm going to mark this as needs work, since webform has lots of submodules with libraries too, and the MR doesn't adjust those.

I also wonder if it would be a suitable approach for webform to use its own constant in the files, like WEBFORM_VERSION and then at runtime do a library alter to swap out that string for the actual webform version, if one can be determined? We don't want all of contrib doing that, but maybe webform could?

steven jones’s picture

steven jones commented
Status: Needs review » Needs work
steven jones’s picture

steven jones commented

More generally, I like the proposed resolution, we might as well set everything to version 1, and then every time the JS/CSS in a library changes, increment that number. There's no need for it to be semver or anything exciting tbh.

steven jones’s picture

steven jones commented

And...if you don't/can't/won't change the contrib versions, we're looking to get a workaround into our helper module that should 'fix' all contrib:
https://www.drupal.org/project/cm_tools/issues/3464016

danchadwick’s picture

danchadwick commented

This is sort of a core problem thrust upon contrib. Once could argue that VERSION in a module or theme should be interpreted as the module's version. So I think we should set aside WEBFORM_VERSION as scope creep. That leaves two good options:

1. Remove all the version properties and rely on caching.
Pro: Super simple.
Con: a tiny bit of extra CPU time when the caches re being rebuild. I assume (without actually checking) that this is very infrequent.

2. Add versions and be sure to update them during commit, checking for conflicts.
Pro: More efficient at cache rebuild time
Con: Requires developers to remember. Possible conflicts if the version is bumped twice before a conflicting merge is made, resulting in re-using an older version number.

I propose we move forward with 1, including removal of all inappropriate versions in webform and all its submodules.

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

dieterholvoet’s picture

dieterholvoet commented
Status: Needs work » Needs review

Rebased and removed the remainder of version: VERSION lines.

snehal-chibde’s picture

snehal-chibde commented

I have checked the above MR where all the version keys are removed and it works fine.
I also agree to @steven_jones opinion and @danchadwick 2nd point, set everything to version 1, and then every time the JS/CSS in a library changes, increment that number. we do not need to rely on cache, or updating the WEBFORM_VERSION with any extra processing.

liam morland made their first commit to this issue’s fork.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Version: 6.2.x-dev » 6.3.x-dev
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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