Make assets:// stream wrapper overrideable

Closes #3504164

I will note that I have seen non local streamWrappers implement getDirectoryPath and return '';

Could this cause a BC issues if they were expecting the core script to fallback to registering at //{file_name} ? (Might be off API and safe to break?)

I think all of what you descirbe is "off API."

The docblock for LocalStream::getDirectoryPath() is not very clear. It reads:

Gets the path that the wrapper is responsible for.

Which is a bit... vague... but I think that means, "gets the path relative to the Drupal installation where this stream wrapper's files are stored on disk." If that's true, and I think it is, then this is a method that is rightly scoped to "local" stream wrappers.

I would imagine if there are non-local stream wrappers implementing this method it's to avoid the underlying issue here. In many cases this would be a no-op, but since we are creating routes based on this path, you do indeed get invalid/not-quite-right paths like //{file_name}.

This all works properly in a "local files" context because there's a bit of a slight of hand going on. The routes registered in AssetRoutes::routes() operates very similar to what we do for image derivatives - if there's not a generated derivative on disk, we create it, but then next time the server looks for that path, it's present on disk and served directly. The current inline docs in AssetRoutes reinforces this:

If clean URLs are disabled image derivatives will always be served through the routing system. If clean URLs are enabled and the image derivative already exists, PHP will be bypassed.

None of that works out of the box if your underlying storage is not persisted to disk in a location the web server accesses via a public URL. Hence this issue.

So yeah, the hacky thing (setting this to an empty string, for instance) generally doesn't matter, but in this case Drupal core makes an incorrect assumption that ::getDirectoryPath() is truly meaningful.

The failure mode here is due to the mismatch between the external URL generated by the stream wrapper for assets:// paths, and the routes for the asset generation controller. For a local driver, that path will match exactly the route from AssetRoutes::routes() by convention. Paths to assets are generated only with assets://, not by referring to the system.[css|js]_assets routes. (Confirmed this by a search of core code.) If the external URL generated for the assets stream wrapper doesn't match exactly the route registered (including a publicly-accessible directory path), the file is never generated because the client doesn't know where to request it. Hence the _drupal_assets path default.

would it be simplest to check for an empty string/falsey value and fallback to the default directory path in this case? We could even trigger a deprecation error

would it be simplest to check for an empty string/falsey value and fallback to the default directory path in this case? We could even trigger a deprecation error

I get where you're going but I don't think this is an appropriate approach for two reasons. One, there is a bit of a BC consideration (as has already been mentioned) for stream wrappers that may, for whatever reason, want this to be "empty." And secondly, we would continue to assume - wrongly - that the asset stream wrapper implements LocalStream. Right now, developers must work around this assumption. Adding this check allows us to avoid the BC issue and make this "correct" instead of continuing to effectively enshrine the false requirement that the stream wrapper implement this method.

Adding this check allows us to avoid the BC issue and make this "correct" instead of continuing to effectively enshrine the false requirement that the stream wrapper implement this method.

Opposing side: If a local disk storage can not extend the core LocalStream class core will not auto-detect this path. This itself is a sign of a bandaid and not a root fix.

My view is that this method either needs to be avoided and some other solution found (using realpath() if it is in the DRUPAL_ROOT a path can be calculated) or it needs to be fully added to API (which means this issue needs to be postponed)

Is there a need for allowing contrib to override this default or is it safe to just have a contrib module de-register the routes AssetRoutes sets and add their own?

I don't think contrib would need to override this unless they don't like the name. In that case, yes they could alter routes.

Take a look at flysystem!86 (111c9379) where we build an external URL for the swapped-out, non-local stream wrapper using the route built from this constant.

The use of getDirectoryPath() is very likely wrong as is, however the real question is there a better method to do this?

Related is, should assets:// be always required implement LocalStream? Is there a performant reason to allow CSS/JS to be on non-local streamWrappers since these can now be generated on demand without need for central storage?

The use of ::getDirectoryPath() is very likely wrong as is, however the real question is there a better method to do this?

I believe this MR is the correct solution.

Related is, should assets:// be always required implement LocalStream?

I think this would be a mistake and a step backwards in supporting a more cloud-native Drupal that can run in contexts with no write-enabled disk storage. See https://www.drupal.org/project/drupal/issues/2724963 (""Make the public file system an optional configuration")

Is there a performant reason to allow CSS/JS to be on non-local streamWrappers since these can now be generated on demand without need for central storage?

Yes, because the architecture of this feature is designed around performance - it just doesn't yet support this variation of backing storage. On one extreme you could theoretically have some type of null storage, where the asset is always generated on-demand - 0% performance improvement. On the other, and this is why I spend a lot of time working on these things, is that your assets are served from a CDN, presumably at an edge cache very close to the user. That's a significant performance improvement for every request following the cold cache hit. For most users, the middle-ground of having your web server serve these files from disk is fast enough. If you don't have any local storage to speak of, it's not currently an option.

There are many ways to skin this cat but I don't think we need to lock ourselves into any particular approach. The great thing about the stream wrapper model is that you can swap out the backing storage any way you like. This is an uncommon (but still valid and not super rare) use case, as evidenced by the fact Flysystem has supported serving asset aggregates for many years. Much of the custom code to do so can now be ripped out of contrib, but core just needs to be a little friendlier to all manner of stream wrappers.

added 60 commits

• 11efb85b...b60186bd - 58 commits from branch project:11.x
• e521a58e - Make assets:// stream wrapper overrideable
• 1cccddda - Back off changes on the asset controller and just focus on the stream wrapper.

Compare with previous version

I think we could write a kernel test using DummyExternalReadOnlyWrapper to validate this is working

left review comments