AssetRoutes::routes() assumes stream wrapper implements ::getDirectoryPath()

Created on 3 February 2025, 10 days ago

Problem/Motivation

AssetRoutes::routes() contains logic to determine where to store "assets," aka CSS and JS aggregates. This used to be hard-coded to use the public stream wrapper, which implements a ::getDirectoryPath() method. When a specific assets:// stream wrapper was introduced in πŸ“Œ Make css/js optimized assets path configurable Fixed , the call to ::getDirectoryPath() was maintained, however if you override the assets stream wrapper (e.g., with Flysystem to point at object storage) this method is not available as it is not in the StreamWrapperInterface contract.

We do not have test coverage for this as it's an expert-mode customization to Drupal, but we should not make assumptions about the stream wrapper that is used.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ› Bug report
Status

Active

Version

11.0 πŸ”₯

Component

file system

Created by

πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life

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

Merge Requests

Comments & Activities

  • Issue created by @bradjones1
  • Merge request !11110Make assets:// stream wrapper overrideable β†’ (Open) created by bradjones1
  • Pipeline finished with Success
    9 days ago
    Total: 776s
    #414950
  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life
  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life

    Major as this blocks alternative implementations for assets:// - "Render one feature unusable with no workaround"

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    Tagging related issue where its been known in the past this method was used when not on an API and should not be used in core.

  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    Can we just use \Drupal\Core\StreamWrapper\StreamWrapperInterface::realpath() ?

  • πŸ‡¬πŸ‡§United Kingdom catch

    The code comments appear to indicate they the alternative stream wrapper is never used to directly serve a file. In which case why change the stream wrapper at all - could override the controller and use a cache backend to store the file contents instead.

  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life

    Thank you all for chiming in on this. I hope my responses in the MR thread addresses much of the questions around the why and what.

    I would like to summarize the reasons for supporting non-local stream wrappers, however. Catch's comment sums up the issue well:

    The code comments appear to indicate they the alternative stream wrapper is never used to directly serve a file. In which case why change the stream wrapper at all - could override the controller and use a cache backend to store the file contents instead.

    There's two main issues at play here. One is the time to generate the asset, which we can all agree is significant and we want to avoid doing so more than once. Second is the question of how and where the client requests the generated file on subsequent requests.

    The first concern is addressed out of the box no matter the local/non-local status of the stream wrapper. The asset controller doesn't find a matching file (loaded with assets:// stream wrapper, works no matter the backing storage), generates it, saves it and sends the result to the client.

    We are addressing here the second issue. It is the stream wrapper's job to yield an external URL for the file - see \Drupal\Core\StreamWrapper\StreamWrapperInterface::getExternalUrl(). For local stream wrappers, this very conveniently matches the paths from AssetRoutes::routes(). For others, the stream wrapper must special-case `assets://` and point the external URL at the asset controllers. (I've linked an example from an MR against Flysystem, above.)

    Subsequent requests for an already-generated asset would be piped, but it still has the overhead of bootstrapping Drupal. This is where the site owner's responsibility lies to have that external URL behind some sort of caching reverse proxy.

    I'm proposing the fix we have here because it 1) doesn't change anything about the default case of using local storage and 2) allows expert site owners to utilize the core asset generation API but in a way that gives you maximum flexibility about the backing storage. For instance, a Drupal application that has many web containers running in a serverless environment wouldn't need to each have its own memory cache copy of the asset.

  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life
  • πŸ‡ΊπŸ‡ΈUnited States bradjones1 Digital Nomad Life
  • πŸ‡ΊπŸ‡ΈUnited States cmlara
  • πŸ‡¬πŸ‡§United Kingdom catch

    @bradjones1 the main reason to prevent sending cacheable headers there is to avoid, for example, different CDN endpoints from serving the PHP version of the response vs. the file as it's served from the web server direct from disk. Even if the content can be guaranteed to be exactly the same, the headers would be different.

    There are two reasons for this apart from consistency:

    I've seen situations in the past (maybe Drupal 7 era) where you end up with different versions of supposedly the same thing in different CDN endpoints and it was not enjoyable. Even something like different max age/expires headers could be annoying.

    Also some hosts, the only example I know of is Pantheon but there may be others, bill differently for requests served from PHP vs static assets from disk, even when those requests are cached in the CDN. I don't understand why they make that distinction but it's how I interpret this documentation:

    https://docs.pantheon.io/guides/account-mgmt/traffic#pages-served

Production build 0.71.5 2024