Make css/js optimized assets path configurable

- the patch looks great for me, seems it works as is in #4
- no worries with variables nomenclature
- i do vote for a UI for the shake of usability :-)

if i'm interested in change assets location i would prefer to specify a given location (in a friendly way, which is always better);
so yes, i would put a markup possibliting this in Performance

Will this be merged to core soon?
Is this amendable via settings.php or only via drush?

OK.. it can be set via

$config['system.file']['assets']['css_base_path']
$config['system.file']['assets']['js_base_path']

I suggest that the patch should provide default directory for those who just apply this patch for the first time e.g. from Drupal 8.6.x. Meaning for those upgrade drupal from composer to not see the broken site. From my understanding, those yml in config doesn't reapply automatically by clearing cache..

Updated for 8.8.x

When trying to apply the patch drupal-3027639-15.patch via Composer in 8.8.x a generic error is thrown saying that the patch could not be applied (but this might be due to permission issues when trying to update the file settings.default.php). After appliying it manually, it seems to update the code properly, however, a warning is displayed by drush when executing cache-rebuild. This warning is way more specific than the composer error and states that AssetsStream::basePath should be compatible PublicStream::basePath. A quick comparison between PublicStream in 8.7.x and PublicStream in 8.8.x shows that the method param type has changed: it no longer uses \SplString as the param type. In conclusion, the patch needs an update to properly work in 8.8.x (or at least, not throwing any warnings when running cache-rebuild via drush). Attached to this comment there is an update that solves the conflict between AssetsStream::basePath and PublicStream::basePath.

It do a digging inside the assets... I am unsure about this patch.. please check the interdiff

coding standards

+++ b/core/lib/Drupal/Core/StreamWrapper/AssetsStream.php
@@ -0,0 +1,62 @@
+    return t('Optimized assets files');

I think we should use StringTranslationTrait and $this->t() instead of t()

Yes StringTranslationTrait makes sense, marking needs work for that.

This seems like a good idea to me, would help when public:// isn't local.

Rerolled the patch in #21 and addressed #25, thanks!

+++ b/core/lib/Drupal/Core/File/HtaccessWriter.php
@@ -107,6 +107,10 @@ public function defaultProtectedDirs() {
+    if ($assets_path != 'public://') {

+++ b/core/lib/Drupal/Core/StreamWrapper/AssetsStream.php
@@ -0,0 +1,64 @@
+    if ($path == 'public://') {
...
+    if ($path == 'public://') {

Strict comparison can be used here

Updated patch in #27 to address #29, thanks!

Please disregard.

Removed the updates to sites/default/default.settings.php as those are already handled in scaffolding.

Updated line 146 in PerformanceForm.php, and added back default.settings.php in sites/default.

Was hoping to be able to serve compressed css and js from a folder different than the default public://css and public://js. My use case is that I am serving content via Akamai simultaneously from two different origins as we migrate the site from D7 to D9. Akamai doesn't know which origin to serve the correct css/js from because they are the same folders on both sites.

Unfortunately, I could not apply the latest patch.

Installing drupal/core (9.4.x-dev efd8f73)
Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2021-10-14/core-optimized-assets-302...

The patch applies to the 9.4x and 10.0.x branches. The commit "9.4.x-dev efd8f73" seems to reference does not exist in Drupal's commit history. What kind of repo is that?

I just sent it out to test again on 9.4.x.

It wouldn't hurt to have some test coverage for this, however there is no test coverage of the writing-to-disk logic in core at all at the moment (we do have tests for lots of other aggregation logic).

There are however some tests in #1014086: Stampedes and cold cache performance issues with css/js aggregation which cover this, and because they're functional tests it would be possible to run them with and without a customised asset directory.

Having a stream wrapper configured to another stream wrapper seems quite unusual and would also be slower, no?

If we really want a stream wrapper for that, could we just make the configuration for it fall back to the same configuration as public:// uses?

FWIW, I don't really understand why we need a stream wrapper for this at all and not just make a setting that defaults to public://? It's contained within those services and not really an API and I'd assume there's almost always going to be another stream wrapper behind it anyway? If you have a use case to put it on S3 for example, you'll need to configure it to use a stream wrapper provided by that integration.

We actually just had a weird use case where this would have been useful. We had a problem with a CDN that had a broken file and it took us a while to clear that. With a setting, we could have switched to a different path and deliver a different filename.

FWIW, I don't really understand why we need a stream wrapper for this at all and not just make a setting that defaults to public://? It's contained within those services and not really an API and I'd assume there's almost always going to be another stream wrapper behind it anyway? If you have a use case to put it on S3 for example, you'll need to configure it to use a stream wrapper provided by that integration.

I think it's the reverse case that's more common. s3fs module can take over the public:// stream wrapper to put all uploads on s3, but you then don't want generated assets on there.

Ok, I assumed that if you have a shared file system you either have multiple web hosts (then the files need to be shared) or no local file system at all.

The path needs to be both a physical path and something that file_create_url() (now url generator) can handle, so I think you could still put sites/default/files in the setting and that would work too. slightly more complicated for the few usages outside of the dumper, but it's arguably bad abstraction anyway that these other services hardcode that path anyway.

either way, atleast we should IMHO not pass the requests through two usercode streamwrappers, which is what is happening now?

Yes removing the stream wrapper doubling sounds good, and if it's possible to remove the stream wrapper but still make it simple to have an alternative local path without relying on public:// that's great.

@catch - #41 is exactly our use case.

I had the same problem as Comment#37. The problem was with the default.settings.php.
I recreated the patch without those changes. Isn't the default.settings.php built from the scaffold file?
My patch works with 9.3.

Updated patch will fix test failure.

> I recreated the patch without those changes. Isn't the default.settings.php built from the scaffold file?

In a composer project, yes. But the git repository has that file and the change needs to be included there, that's where the scaffold feature gets the files from.

Here's a re-roll, also changing the fallback from public:// to Settings::get('file_public_path') to avoid the stream wrapper nesting. That should allow this patch to fix #3323934: Breaking changes for public:// takeover in D10.1 from #1014086 without any configuration on the part of the site owner.

More of a task.

+++ b/core/lib/Drupal/Core/StreamWrapper/AssetsStream.php
@@ -0,0 +1,63 @@
+    $path = Settings::get('file_assets_path', 'public://');
+    if ($path === 'public://') {

What is this special case for? The docs say file_assets_path should be a local path, not a URI.

edit: if it's only for the default/unset case, why not use NULL?

I'd missed a couple of public:// - uploading a new patch that addresses those.

The logic is that if the path isn't set, or if it's been explicitly set to exactly the same as the public file path, then we want it to work the same as the public stream wrapper - so it's try to preserve the ability to explicitly set the public files base URL.

I think it would be possible to use NULL and fall back too, but setting explicitly the same directory does seem like something that could happen.

Since the storage path is configurable should we also have \Drupal\system\Routing\AssetRoutes::routes track the path?

I believe this will also create the same issues described in #2979669: Follow-up for #2752961: automatically deleting compiled Twig templates across multiple webheads regarding cleaning up files. We might want to start formally documenting these paths and their recommended cleanup schedules somewhere?

This should cover the first part of #59.

Multi-webhead files clean-up, maybe a new meta issue (and link a follow-up from that)?

Cleaning up stray ')' from patch #57/#59

I'm ok with multi-webhead being a new meta issue.

Developers of contrib modules must start using the new stream wrapper:

Is there a way we can help developers discover this, with a deprecation somewhere? Is there any contrib that will be broken on sites that use this new feature, but that is expecting assets in the old location?

(example search https://git.drupalcode.org/search?group_id=2&scope=blobs&search=public%3...)

Had a quick look:

responsive_menu uses the stream wrappper, but it's just putting its own file in the directory, not actually interacting with assets at all, so it wouldn't matter:

breakpoint_css_filepath: public://css

css_flipper does similar.

So does gmap with public::/js

So these are all OK - they're just using those folders names as 'places css or js are written to', not interacting with the aggregates at all.

I also manually checked advagg module, and that does use public:// . $asset_type (which of course doesn't come up in the search) and will likely break, however advagg closely alters the internals of the aggregation system, so, either it'll continue working if it alters enough to essentially override all the core logic, or it will already need to update for 10.1 as per #3308099: Document which parts of the module are still relevant after aggregation changes in 10.1.0.

So overall, we should actually be pretty clear in terms of disruption, and this issue will simplify/fix s3fs module too.

The code so far looks good, but this new feature needs test coverage, and we also should mention it in the release notes.

Added a change record and release note.

Added some test coverage. We have various implicit functional testing of the unconfigured fallback (such as in Umami tests), so changed the existing dedicated functional test to test the configured version. This caught some extra references to public:// that needed updating since other aggregation changes in 10.1 too.

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.

Manually testing works, so it's getting the stream wrapper to pick up the setting in tests.

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 #1014086: Stampedes and cold cache performance issues with css/js aggregation.

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 #3323934: Breaking changes for public:// takeover in D10.1 from #1014086, 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 #1014086: Stampedes and cold cache performance issues with css/js aggregation 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 #1014086: Stampedes and cold cache performance issues with css/js aggregation, 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: #3347181: 10.1.x compatibility: tests are failing against Drupal 10.1.x due to upstream changes … and it definitely looks plausible that #1014086: Stampedes and cold cache performance issues with css/js aggregation 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 #3179753: Improve far-future support: generate dynamically generated files automatically (f.e. image style derivatives) 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 #2670404: Add new stream wrapper(s) to store generated files separately, 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 New assets:// stream wrapper for custom aggregate path location — 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 #3323934: Breaking changes for public:// takeover in D10.1 from #1014086) 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.

#3298701: ImageStyleDownloadController routes do not limit schemes served 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 assets:// 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?

Edit: it’s assets://, plural.

#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 #3354090: Better default base path in assets stream wrapper. Given the visibility flag might need more discussion, going to split that out to and move this back to fixed. See you on #3354091: Asset stream wrapper may need to be selected in the UI for some cases/

Also opened #3354207: Allow image style derivatives to use a separate stream wrapper.

#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 #3354204: [regression] Since #1014086 generated CSS assets have absolute URLs without varying by url.site cache context.

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 #3354091: Asset stream wrapper may need to be selected in the UI for some cases!

Don't we also want a follow-up for image derivatives? I guess that needs its own stream wrapper based on #94's feedback? cross-posted with #96 — following!

Hello,
I would like to separate JS, CSS and PHP directories in the public files, into a "sites/default/files/assets" folder, for example.

I've used, in Drupal 10.4.5, $settings['file_assets_path'] but it does'nt works: no CSS/JS/PHP files are created, even after cache_rebuild.

Not sure that all the used "assets://css" or "assets://js" are found.
When I load a page, the css, js or oho folders aren't created.

How does $settings['file_assets_path'] is supposed to work please ?

Thank you by advance for an answer.

Regards

FYI, I opened a follow up issue
#3591076: JS translation files should be generated and served from assets://