CssCollectionOptimizerLazy::generateHash doesn't allow for absolute paths in assets

Created on 29 September 2023, 11 months ago
Updated 3 October 2023, 11 months ago

Problem/Motivation

CssCollectionOptimizerLazy::generateHash calls file_get_contents on the asset path if the version is not set.
We support absolute paths in these urls so you end up with a file_get_contents on a non-existent path.

Steps to reproduce

Declare a libraries.yml like so, note the leading /

global:
  css:
    base:
      /libraries/css/base.css: { minified: true }

View a page with CSS aggregation turned on.
Check logs and find

Warning: file_get_contents(libraries/css/base.css): Failed to open stream: No such file or directory in Drupal\Core\Asset\CssCollectionOptimizerLazy->generateHash() (line 43 of core/lib/Drupal/Core/Asset/AssetGroupSetHashTrait.php).

Proposed resolution

Mirror the logic in \Drupal\Core\Asset\LibraryDiscoveryParser::buildByExtension that deals with items that start with a leading / into CssCollectionOptimizerLazy::generateHash

           if ($source[0] === '/') {
              // An absolute path maps to DRUPAL_ROOT / base_path().
              if ($source[1] !== '/') {
                $source = substr($source, 1);
                // Non core provided libraries can be in multiple locations.
                if (str_starts_with($source, 'libraries/')) {
                  $path_to_source = $this->librariesDirectoryFileFinder->find(substr($source, 10));
                  if ($path_to_source) {
                    $source = $path_to_source;
                  }
                }
                $options['data'] = $source;
              }
              // A protocol-free URI (e.g., //cdn.com/example.js) is external.
              else {
                $options['type'] = 'external';
                $options['data'] = $source;
              }
            }

Remaining tasks

Tests, fix

User interface changes

API changes

Data model changes

Release notes snippet

πŸ› Bug report
Status

Closed: works as designed

Version

10.1 ✨

Component
Asset libraryΒ  β†’

Last updated 1 day ago

No maintainer
Created by

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @larowlan
  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    Temporary workaround

    
    /**
     * Implements hook_library_info_alter().
     */
    function YOURTHEME_theme_library_info_alter(array &$libraries, string $extension): void {
      if (!in_array($extension, ['SOME_THEMES', 'YOU_CARE_ABOUT'], TRUE)) {
        return;
      }
      $deploymentIdentifier = Settings::get('deployment_identifier', \Drupal::state()->get('system.css_js_query_string', 0));
      foreach ($libraries as $key => $library) {
        if (!array_key_exists('version', $library)) {
          $libraries[$key]['version'] = $deploymentIdentifier;
        }
      }
    }
    
  • πŸ‡¬πŸ‡§United Kingdom catch

    Proposed resolution makes sense to me.

  • πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

    Hm, strange, we have plenty of libraries like this and didn't run into this issue. dropzonejs module, for example. Anything else going on here?

  • πŸ‡¨πŸ‡­Switzerland Berdir Switzerland

    Ah, dropzonejs does have a version and so do probably most that are using /libraries, so I assume that's the issue. Sorry for the noise, ignore me and move on :)

  • Status changed to Postponed: needs info 11 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    I spent some time trying to reproduce this, and can't.

    I tried to write some test coverage for this, and it passed locally.

    Then I tried to reproduce manually.

    If I put a URL like '/libraries/foo.css', then it works whether the libraries directory is in the root directory or in 'sites/default/libraries', this is because AssetGroupSetHashTrait already operates on assets that have been processed by \Drupal\Core\Asset\LibraryDiscoveryParser::buildByExtension

    If I put a URL like '/core/misc/assets/foo.css' then that same logic converts it to a relative url 'core/misc/print.css' (the substr($source, 1) call).

    The only way I can get file not found is to put a file that actually doesn't exist in, which I did just to make sure my changes were being picked up.

    All I can think is maybe this isn't coming from the raw library definition but maybe an alter somewhere, but then that needs much more specific steps to reproduce.

    For reference both of these work:

    diff --git a/core/modules/system/system.libraries.yml b/core/modules/system/system.libraries.yml
    index 2f510872d5..39c3edc0e1 100644
    --- a/core/modules/system/system.libraries.yml
    +++ b/core/modules/system/system.libraries.yml
    @@ -1,5 +1,4 @@
     base:
    -  version: VERSION
       css:
         # Adjust the weights to load these early.
         component:
    @@ -12,7 +11,7 @@ base:
           css/components/details.module.css: { weight: -10 }
           css/components/hidden.module.css: { weight: -10 }
           css/components/item-list.module.css: { weight: -10 }
    -      css/components/js.module.css: { weight: -10 }
    +      /libraries/foo.css: { weight: -10 }
           css/components/nowrap.module.css: { weight: -10 }
           css/components/position-container.module.css: { weight: -10 }
           css/components/progress.module.css: { weight: -10 }
    
    diff --git a/core/modules/system/system.libraries.yml b/core/modules/system/system.libraries.yml
    index 2f510872d5..4c14d716cc 100644
    --- a/core/modules/system/system.libraries.yml
    +++ b/core/modules/system/system.libraries.yml
    @@ -1,5 +1,4 @@
     base:
    -  version: VERSION
       css:
         # Adjust the weights to load these early.
         component:
    @@ -12,7 +11,7 @@ base:
           css/components/details.module.css: { weight: -10 }
           css/components/hidden.module.css: { weight: -10 }
           css/components/item-list.module.css: { weight: -10 }
    -      css/components/js.module.css: { weight: -10 }
    +      /core/misc/print.css: { weight: -10 }
           css/components/nowrap.module.css: { weight: -10 }
           css/components/position-container.module.css: { weight: -10 }
           css/components/progress.module.css: { weight: -10 }
    
  • last update 11 months ago
    29,643 pass
  • @catch opened merge request.
  • πŸ‡¬πŸ‡§United Kingdom catch

    Opened an MR with the extra (minimal) test coverage but leaving at needs more info.

    I'm wondering if this was an alter hook adding something from the libraries directory, but without determining where the libraries directory actually is - however since it's altering information where that has already been resolved, I'm not sure core should be resolving it again just in case - we'd be running exactly the same logic twice then. Instead the alter code could resolve the libraries location and use that.

    If I'm guessing correctly what the problem is at all of course.

  • Status changed to Closed: works as designed 11 months ago
  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    Yep, re-investigated this and you're right, the file was missing on disk

    Sorry for the runaround

Production build 0.71.5 2024