Make css/js optimized assets path configurable

For the new file AssetsStream can we typehint the new functions?

Applied patch on Drupal 10.1 with a standard install
Verified going to admin/config/media/file-system I see

Optimized assets file system path
sites/default/files

Updated my settings.php with $settings['file_assets_path'] = '../test';

Verified file-system page updated correctly

When I turn preprocess back on

$config['system.performance']['css']['preprocess'] = TRUE;
$config['system.performance']['js']['preprocess'] = TRUE;

The pages completely broke.

Did I miss a step? Nothing gets added to test folder

Addressed type hints for the AssetsStream class #67, added review comments in MR. Hide the files.

Did I miss a step?

Did you do a full cache clear? The router needs to be rebuilt so that the routes serve the assets at the newly configured path.

Cleared cache multiple times.

Updated my settings.php with $settings['file_assets_path'] = '../test';

I missed this the first time I read it - the path has to be inside the webroot so that Drupal can take over routing, so ../test won't cut it. You could change it to something like 'files/assets.

@catch Yes, this would have to be inside the web root since the resulting files are served by the web server directly (Apache, nginx) rather than going through Drupal like in the case of private files. We use this on Omnipedia with .$settings['file_assets_path'] = 'assets'; which results in aggregated assets being placed in https://omnipedia.app/assets/; the docblock in the merge request's settings.php clearly states this:

/**
 * Optimized assets path:
 *
 * A local file system path where optimized assets will be stored. This
 * directory must exist and be writable by Drupal. This directory must be
 * relative to the Drupal installation directory and be accessible over the
 * web.
 */

When in doubt, read the docblock. 😉

Retested patch #65
Created a folder web/files/assets.
Turned
$config['system.performance']['css']['preprocess'] = TRUE;
$config['system.performance']['js']['preprocess'] = TRUE;
Cleared cache

Verified assets were being placed in the folder. Page was rendering fine.

In the test change, the setting name is wrong. I think we need to add an assertion somewhere in the test to ensure the path to a generated asset is actually what we expect.

I started working on fixing the test coverage.

Now fails when the asset URL doesn't match what we think we're setting it to.

Unfortunately I don't seem to be able to get ::writeSettings() and ::rebuildAll() to successfully change what the asset stream points to yet, so it will actually fail.

catch → credited olli → .

Test now asserts that the URL is what it should be, and the reason it was failing was because I missed the extra 'settings' key in the writeSettings() format as pointed out by @olli in the MR. Back to needs review now it's green again.

Tagging as a Drupal 10.1 target since this will make things easier for s3fs (and possibly other contrib stream wrappers) after 🐛 Stampedes and cold cache performance issues with css/js aggregation Fixed .

Tested same way as before (#74) and still seems to be working. Don't mind marking.

Committed and pushed to 10.1.x, and published the change record. Thanks!

Woah, had no idea this was happening!

From the perspective of modules like https://www.drupal.org/project/cdn → , this is a BC break, because suddenly assets have different URIs (no more public://something but assets://something. I'm happy to make sure that that module is updated, but I think the change record should be more explicit and inform contrib/custom modules that they will need to apply the treatment they had for public:// for assets:// instead now.

Per 🐛 Breaking changes for public:// takeover in D10.1 from #1014086 Closed: outdated , this is also a breaking change for the https://www.drupal.org/project/s3fs → module. But what isn't clear to me is how the CDN module will detect that assets:// is actually on S3. I think that means that the S3FS module should override the entire AssetsStream class to not do class AssetsStream extends PublicStream extends LocalStream anymore.

So the CDN module (and others) would have to detect at runtime whether the current class is an instance of LocalStream or not. That seems like a risk and something that we're lacking documentation for, especially if the very purpose of this new stream wrapper is to be overridden? 😅

@Wim

From a s3fs standpoint, I had anticipated we would treat assets:// similar to temporary://, its there, it exists, its per server, and we don't modify it. The newest worst case is that each edge server has its own copy of CSS files however these are small and per 🐛 Stampedes and cold cache performance issues with css/js aggregation Fixed are quick to dynamically generate. As long as core doesn't start using assets:// for for files that can not be reproduced on demand (or are 'expensive' to produce) its safe for a multi-headed environment and s3fs doesnt need to take it over.

I suppose someone could submit a feature request sometime in the future that s3fs support being storage for assets:// as as well, however if they did so, I wonder if it would it be much different for CDN compared to when the s3fs modules replaces the public:// and private:// streamWrapeprs with s3 based storage?

Yes the issue with s3fs and public files takeover vs. asset aggregation is that it breaks the routing that Drupal needs to generate the aggregate. Now that assets:// is it's own stream wrapper, s3fs can separate the treatment of assets:// and public:// and leave things alone.

I've used s3fs and public files takeover on at least one site, and the use case has been a site hosted somewhere with a somewhat restrictive amount of file storage (pantheon, platform.sh etc.) where you want lots of uploaded files on s3 instead for storage. For these, moving assets:// back to the Drupal server is ideal. I've been running this patch in production on at least one 9.5 site, and it works fine without any changes to s3fs at all - just to avoid the URL rewriting that s3fs does.

For CDN module, if nothing needed to change for 🐛 Stampedes and cold cache performance issues with css/js aggregation Fixed , then it is probably just pushing the files up to the CDN from assets:// that needs to be covered - however I'm not sure how CDN module handles image derivatives and now route-generated asset aggregates, been a little while (several years) since I looked tbh.

I've updated the change record - @Wim Leers does that help?

https://www.drupal.org/node/3328126 →

This can't be backported, not sure if you meant to mark needs work, but moving back to fixed for now.

High-level impact on contrib modules

The CDN module is not yet working on Drupal 10.1: 📌 10.1.x compatibility: tests are failing against Drupal 10.1.x due to upstream changes Fixed … and it definitely looks plausible that 🐛 Stampedes and cold cache performance issues with css/js aggregation Fixed caused that.

however I'm not sure how CDN module handles image derivatives and now route-generated asset aggregates, been a little while (several years) since I looked tbh.

The Drupal 8/9/10 version of the CDN module (version >=3) does not handle image derivatives, because there used to be no way to do that reliably. Work is happening at ✨ Improve far-future support: generate dynamically generated files automatically (f.e. image style derivatives) Needs work to restore that. And you hit the nail on the head: if I want to support that, I'd much rather have that work generically for any route that generates assets rather than only image derivatives.

Why only CSS & JS assets, and not also image derivatives, i.e. all "optimized assets"?

Reading through that made me re-discover ✨ Add new stream wrapper(s) to store generated files separately Needs work , which is a very closely related core issue. That issue is basically identical to this issue, except that it proposed to do it not only for CSS & JS aggregates, but all generated assets, including image derivatives. And in fact, assets:// sounds generic enough that it could really be used for any kind of asset (even generated an non-generated?). The change record makes this rather clear to: it's titled — it needs that "for …" part to clarify what it's intended to be used for. The code speaks about "optimized assets", which would include image derivatives too, wouldn't it?

Regression preventing contrib modules from working correctly

I've updated the change record - @Wim Leers does that help?

Not quite yet. Because the problem of "local vs not" is not yet addressed.

You refer to \Drupal\Core\StreamWrapper\TemporaryStream, but that has StreamWrapperInterface::LOCAL_HIDDEN as its type: always local, always hidden. That makes sense.

But AssetsStream says return StreamWrapperInterface::LOCAL_HIDDEN too. This is a problem because: the documentation says:

  /**
   * Refers to a local file system location.
   */
  const LOCAL = 0x0001;
…
  /**
   * Exposed in the UI and potentially web accessible.
   */
  const VISIBLE = 0x0010;

… CSS & JS aggregates surely are always web accessible? 😅 And then depending on whether S3FS's "takeover" functionality (see 🐛 Breaking changes for public:// takeover in D10.1 from #1014086 Closed: outdated ) is used or not, it may be local or not. That's the problem I was referring to in #85.

That's a problem because it prevents modules like the CDN module from providing configuration, choices and behavior to the user based on the stream wrapper's metadata:

1. \Drupal\cdn_ui\Form\CdnSettingsForm::buildForm() allows choosing to enable the CDN for any VISIBLE stream wrapper (since #2870435: Support additional stream wrappers → ):
   
2. … in this new world where stream wrappers are actually fairly likely to have their type change due to modules like S3FS swapping out the implementation, I'll have to write a new validation constraint to verify that the stream wrappers for which the CDN module is enabled is actually one of the VISIBLE type

Regardless of the new challenges for the CDN module (I'm happy to overcome those! 😊 It's great to see progress on this front! 🤩), I think this is a plain bug:

  /**
   * {@inheritdoc}
   */
  public static function getType(): int {
    return StreamWrapperInterface::LOCAL_HIDDEN;
  }

it should be:

  /**
   * {@inheritdoc}
   */
  public static function getType(): int {
    return StreamWrapperInterface::READ_VISIBLE;
  }

… because it's A) definitely web accessible and readable, B) even if it's local by default, it's just as likely not to be, so it's better to treat that aspect of this stream wrapper as a black box and hence omit WRITE and LOCAL. If you disagree with point B, then it should be StreamWrapperInterface::READ_VISIBLE | StreamWrapperInterface::LOCAL.

Why only CSS & JS assets, and not also image derivatives, i.e. all "optimized assets"?

I've wanted this as well for image derivatives, also due to s3fs public takeover - i.e. it would allow you to have original PDFs + images on s3, but avoid a lot of complexity for image derivatives (no URL rewrites etc.). Also for a lot of configurations it would mean that page assets save the extra DNS request if you're serving from a files.example.com domain etc.

However for the purposes of this issue was doing 'one thing at a time'.

It would be straightforward to add a follow-up for image derivatives, but it also means the meaning of the configuration would change if we make it also apply to images... so should we combine everything in here?

It would be straightforward to add a follow-up for image derivatives, but it also means the meaning of the configuration would change if we make it also apply to images... so should we combine everything in here?

I would be concerned there is actually more complexity involved and that moving ImageStyles to assets:// should be given a separate in depth discussion.

The biggest issue that comes to mind is that currently the asset:// streamWrapper must store files inside the docroot, however image style derivatives of private:// (and other non-public streamWrappers) should not be inside the docroot. That by itself may make asset:// unsuitable for storing all image styles.

Beyond that image styles are a different cost equation (bandwidth transfer. computational cost, local disk storage, etc) compared to css/js file creation and should be given a much deeper discussion around DoS potentials, especially as you start discussing multi-server with load balancing proxies. We may very easily significantly weaken the ITOK protection system.

My personal opinion based on recent work with the ImageStyle generation system is there are 'limitations' in the current design, and a centralized asset:// storage location may or may not make the issues worse depending upon actual implementation.

All of the above makes me believe that 'take it slow and handle it separate' is a good idea as this comes to ImageStyles.

🐛 ImageStyleDownloadController routes do not limit schemes served Fixed is also related to ImageStyle storage changes.

From an s3fs standpoint:
I've certainly seen some requests to move ImageStyle storage away from the S3 bucket to local storage, that would certainly appease some of the s3fs user base, and would allow us to remove some areas of complexity we currently have in the code. I certainly have some concerns about impact and attack vectors that trying to generate ImageStyles from remote sources necessitate design implementations considerations, See #3298703: Core ImageStyleDownloadControler allow DoS for s3fs. →

#91: You're right this cannot work for private://. But \Drupal\image\Entity\ImageStyle::fileDefaultScheme() currently returns either private or public. What would the harm be in making that private (unchanged!) or assets? 🤔

#90: I think the configuration that this added already is fine precisely because it's so abstract: it says "optimized assets". Images are assets, so … I don't see what needs to change? It says that everywhere: in settings.php, in FileSystemForm, and so on. That's why I'm confused why it wasn't applied to image derivatives too! 😄 But yes, doing that in a follow-up definitely works 👍 My primary concern is the use of LOCAL_HIDDEN.

I think we should actually use the same as PublicStream and stop overriding the method, which is LOCAL_NORMAL, which looks pretty consistent with #89 just a different way of doing it.

I also noticed there was a subtle change in how ::basePath works compared to public, so adding a change for that too.

If this is green and RTBC, then maybe we can quick fix it, but otherwise I think we should either roll back and recommit with the changes or open a (hopefully quick) follow-up given we're over 90 comments here.

Let's open a follow-up for image styles - we could potentially add it to assets:// if we can figure out how to transparently, or add image_styles://

Images are assets, so

What I think we are discussing here is Drupal Language vs Plain English and its impact on the users. Its my understanding that anything created under one of the *AssetOptimizerInterface are "Assets", anything else is not. Perhaps including some wording along those lines would clear this up to avoid future confusion that Assets are only those files created by Asset Aggregation.

I think we should actually use the same as PublicStream and stop overriding the method, which is LOCAL_NORMAL, which looks pretty consistent with #89 just a different way of doing it.

Doesn't it being flagged LOCAL_NORMAL make it available in the Drupal FileSystem UI for every file field as a destination for files? It could even be configured as the Default storage location. Do we have enough verbiage in the display to discourage the use of the scheme for storing average files?

Note: This isn't a no vote, just want to make sure the consideration of the repercussions of the VISIBLE flag are considered as to what it means for most of the Drupal UI.

@Wim Leers
I know this helps s3fs significantly so I would hate to see a revert if we can't come to an agreement on visibility.

Are you closer to supporting the new D10 system with asset:// being present or being reverted? Is this perhaps really an issue that the VISIBLE key isn't the right configuration key to use in the CDN UI and maybe the right key just doesn't exist yet? Is there a future where we might have more streamWrappers that we want to use for core or contrib but don't want to be visible in the UI normally yet CDN might still need to modify them?

#94 is a good point - there's no 'UI' constant, VISIBLE does double duty for 'accessible via web' and 'shown in the UI'. I don't think we want people configuring fields to point to this, so not sure what the best option is. Could CDN module check the class as a workaround? That wouldn't help when it's swapped out, but it would when it's just configured using the core class, and alternative implementations could change the visibility flags.

Split the base path issue to 🐛 Better default base path in assets stream wrapper Fixed . Given the visibility flag might need more discussion, going to split that out to and move this back to fixed. See you on 🐛 Asset stream wrapper may need to be selected in the UI for some cases Active /

Also opened 📌 Allow image style derivatives to use a separate stream wrapper Closed: duplicate .

#93: That works for me 👍

#94:

Its my understanding that anything created under one of the *AssetOptimizerInterface are "Assets", anything else is not.

Oh interesting, that's another reasonable way of looking at it, and that'd completely invalidate my reasoning of "CSS and JS assets are assets, but so are image derivatives".

Are you closer to supporting the new D10 system with assets:// being present or being reverted?

Yes, but I found a real regression blocking it 🫣 See 🐛 [regression] Since #1014086 generated CSS assets have absolute URLs without varying by url.site cache context Fixed .

Could CDN module check the class as a workaround? That wouldn't help when it's swapped out, but it would when it's just configured using the core class, and alternative implementations could change the visibility flags.

I can hardcode the exemption of assets:// despite it being VISIBLE. That's a pragmatic work-around for now. 👍 See you in 🐛 Asset stream wrapper may need to be selected in the UI for some cases Active !

cross-posted with #96 — following!

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