Async css/js creation from #1014086 means there is no efficient way to get CSS from a library

This is a behaviour change that is potentially disruptive for modules that are altering the way asset aggregation (or the public:// URL scheme) works, which is why it's in a minor release and not a patch release, but it's within the bc policy in that the API for aggregation hasn't changed except the new deprecations. It's in the same rough area as if we change a form or controller that a contrib module is altering - we might break a specific alter hook, because the structure of the form as such isn't included under the public API promise. See https://www.drupal.org/about/core/policies/core-change-policies/bc-policy

I've responded on #3323934: Breaking changes for public:// takeover in D10.1 from #1014086 but to briefly repeat here, I think the best solution for s3fs would be #3027639: Make css/js optimized assets path configurable so that you can have s3fs public:// takeover without needing to alter the asset aggregation logic at all.

There is #3308099: Document which parts of the module are still relevant after aggregation changes in 10.1.0 and #3030974: Support optimized assets path configurable open for advagg which is the other module likely to be most affected. I think we can close this.

This affects Drupal Symfony Mailer (now used on 11k sites), causing major bug #3371042: Drupal 10.1.0 new aggregation breaks InlineCssEmailAdjuster, apparently making the module unusable on D10.1. Probably swiftmailer module is the same as it has similar code.

The key code is this (in InlineCssEmailAdjuster):

    foreach ($this->assetResolver->getCssAssets($assets, TRUE) as $file) {
      $css .= file_get_contents($file['data']);
    }

With #1014086: Stampedes and cold cache performance issues with css/js aggregation, the files don't exist yet and so file_get_contents() fails.

The issue has a patch, but there are some problems:

1. It requires code that checks \Drupal::VERSION, which we shouldn't have to do given core BC policy.
2. It likely makes performance significantly worse, because the patched code entirely ignores the disk-based aggregate, and regenerates it for each email.

Maybe what we need is an extra parameter on getCssAssets() that can be used to force generation of the assets? Older versions of Drupal would ignore the extra parameter.

I want to add my few cents to this. The current CSS/JS aggregation is using similar approach to Image Style functionality. Which means, you can generate a result URI, but the actual file might not exist. Not always images, processed by Image Style, are requested by an HTTP request. For example, when generating some kind of file (PDF, XLSX etc.) that should include an image processed by Drupal, the libraries usually expect a relative path to the image, when they simply check for file_exists() and embed if it does, or skip if a file is missing. They don't make any suggestions how these files are created. And you definitely don't want to provide URLs for that files, which will trigger HTTP request on each image and slow down the process very badly. This is a non-issue for Image Styles, because they provide two public APIs:

• \Drupal\image\ImageStyleInterface::buildUri() — build URI, but file might be missing. This is basically how is now CSS/JS aggregation works.
• \Drupal\image\ImageStyleInterface::createDerivative() — generates an image derivative on call. Calling ::buildUri() after that will lead to already existing image processed by Drupal.

So, new CSS/JS aggregation system basically lacks of ::createDerivative() analogue. The main logic is in \Drupal\system\Controller\AssetControllerBase::deliver().

I think \Drupal\Core\Asset\AssetOptimizerInterface should require a new method implementation that will generate that files on demand.

So, new CSS/JS aggregation system basically lacks of ::createDerivative() analogue. The main logic is in \Drupal\system\Controller\AssetControllerBase::deliver().

I think \Drupal\Core\Asset\AssetOptimizerInterface should require a new method implementation that will generate that files on demand.

I think this would be a good new issue to open.

That sounds like a good approach.

Is there any way to avoid contrib code needing to test on Drupal version to decide whether to call the new method? Could we add a null version of the new method to older core versions??

We can't retrospectively fix older versions that have already been released, so you would still need some checks. We don't add API to patch releases. The safest way is to add method_exists check I think.

Re-titling, I think we can re-purpose this issue.

So: add a new method, which does most of the same logic as AssetControllerBase::deliver() (but doesn't check against the actual request, because it's for situations where you don't have a request or don't want to rely on it, like in Symfony Mailer).

It would be something like

function createAggregate(string $file_name, string $asset_type, array $query): string {}

$query is probably the results of $query->All() and we check against the array instead of using $query->has() - that way calling code that's not the controller can use parse_url to get the query string to pass in.

We can't add a new method to AssetCollectionOptimizerInterface because it both doesn't have a base class and isn't 1-1. Unless we add a base class, which maybe we could do, but we'd also need to add that to the two deprecated implementations or implement the new method on those too.

The other option is a new service with a new interface with a single method.

Either way we can use this service in AssetControllerBase so it'll be used by core.

Although the method is called createAggregate, it'll check for the file if it already exists to avoid creating it again.

Here's a summary of my understanding:

This issue affects any module that uses theming outside of the HTML context. That includes for emails, and could also include for PDF. It affects swiftmailer and symfony_mailer which already means 50k sites.

Apparently there's no way to disable the new aggregation, so these modules currently don't work at all with D10.1. The modules need to change their code, however the new function they need to call doesn't exist yet.

Problems:

1) Documentation/communication: I can't find much indication of this problem in either the release notes or the change records.

2) Action needed: Support for D10.0 ends on December 11th. Before then we need this patch to land, then time for the contrib modules to adjust their code and create a new release. This issue is apparently unassigned with nobody working on it. Can we make it a D10.2 release blocker? Otherwise 50k sites are left stranded with no version of Drupal that's usable and secure.

Thanks😃

A work around for now in 10.1 (for those stumbling on this) is to set the css/jss library with a preprocess: false. That then means the file can be found in its un aggregated form. Not suggesting that's a good or bad solution. But it does seem to work in the mean time.

Actually it's not as many as 50k sites because it's only those that add CSS to the email. Maybe 10k sites?

Seeing as this is an API change then I guess the deadline for 10.2 is the first beta release, Week of November 13, 2023.

A work around for now in 10.1

Thanks. Are there any possible disadvantages? Do you know if we can still do optimization to process @import?

Seeing as this is an API change then I guess the deadline for 10.2 is the first beta release, Week of November 13, 2023.

If this was still not fixed by then and blocking contrib, we'd probably commit it during beta still, maybe not during rc.

I think the main question here is where to put the new method per #3325118-11: Async css/js creation from #1014086 means there is no efficient way to get CSS from a library. Another option I didn't think of when writing that comment was adding a public non-interface method to Css/JsAssetCollectionOptimizerLazy - contrib would need to do method_exists() but that'll be the case for a while even if we were to add a new interface method.

I started looking at adding a helper service which would take the information from the URL and write out a file, and I think it is the wrong approach.

The controller is very specifically dealing with a URL - i.e. it has to get information from the request and verify it, mock the active theme etc., this is because we don't have control over who visits the URL. It also has to do a load of work to parse out the libraries etc. from the URL due to compression etc. it's very, very specific to the fact we have to pass all that information in a URL, the actual code to build the aggregate from the libraries is minimal and using services that core already provides.

The original code in Symfony mailer is building a URL in order to get the contents from it, which is not an API as such but relying on the behaviour, however it already has access to the libraries it wants to get assets for in the first place, which is not the case for the controller at all.

On the other hand, #3371042-24: Drupal 10.1.0 new aggregation breaks InlineCssEmailAdjuster uses core's existing API methods to get the CSS contents from the libraries, and this would have been the correct approach in the first place for earlier Drupal core versions too. The only reason to build a helper to get those contents from a file, would be essentially to use the file as a cache, but since it's the contents of the file that are needed, not the file itself, Drupal's cache system will work fine for that.

The original reason this issue was opened has been resolved via the assets:// stream wrapper.

I'm therefore marking this by design and will update the other issue.

@catch The problem hasn't gone away so I'm re-opening this and will reference it from the Drupal Symfony Mailer release note. Contrib modules such as Drupal Symfony Mailer need an efficient way to get CSS from a library. Prior to #1014086: Stampedes and cold cache performance issues with css/js aggregation, such a way existed, using file_get_contents.

I understand your point that this was "relying on the behaviour", and that as such it might change in a minor version. However I would have thought that in this case it was very important to have a release note informing contrib code, showing the old code and replacement code. I have check the 3 linked release notes and as far as I can tell this didn't happen.

If someone had tried to write this release note it would have revealed the more serious problem: there is apparently no equivalent simple code available.

#3371042: Drupal 10.1.0 new aggregation breaks InlineCssEmailAdjuster has made a fix using this commit. The new code doesn't include any caching, which will create a performance decrease for bulk sending, such as large newsletters. We don't yet have any data however I'm just creating a beta release so hopefully people will report back soon.

In #11 you changed the title to "Add an API method to get the contents of a CSS or JavaScript aggregate from a URL". I trust your expert view in #16 that this actually isn't the correct approach, and instead it belongs in Drupal's cache system. I'm happy to alter Drupal Symfony Mailer for any usable API that Core might provide.

this would have been the correct approach in the first place

Drupal's cache system will work fine for that.

You seem to be implying that the contrib code was wrong in the first place. However the only alternative I can see is to attempt to duplicate the entire Drupal Core CSS caching algorithm in contrib. Therefore I expect that 99% of developers would also have written the same "wrong" code😃.

Already the code after #3371042 is slightly more complex (8 lines rather than 2), duplicating the internal logic in CssOptimizer::optimize(). If we were to add caching then it would likely be more like 80 lines of complex code that is performance critical, possibly security-sensitive and easy to get wrong. We need to calculate a valid unique cache key. We would have to ensure that caches are invalidated when versions change (including complex edge cases such as #3370828: Ensure that edge caches are busted on deployments for css/js aggregates that even Core took several months to figure out) and when caches are manually cleared. And such code would be nearly impossible to maintain as already we have clearly seen that Core can change it substantially in a minor release. Contrib code needs to work with a range of minor releases, so it's just not going to work well.

I have no interest in maintaining such code in Drupal Symfony Mailer😃. If @catch decides to close this issue again, then I guess anyone who has a site that requires it should create their own contrib module. That module could easily implement a Drupal Symfony Mailer plug-in to operate without needing any changes to Drupal Symfony Mailer.

You don't need to worry about edge cases like browser and edge caching of URLs because you're generating CSS for emails, not a URL. Using the cache API will result in invalidation when caches are cleared, which is enough.

So maybe 10-15 lines of code, not 80.

Thanks for the reply @catch. I don't understand what more info you are looking for.

AFAICS we do need to worry about the version field and the special significance of the value -1. Perhaps if you or another expert could write the "missing" change record including the correct lines of replacement code then it would help.

Even then, you didn't respond to the matter of maintainability. Would you be willing to make a commitment to the suggested code remaining correct and complete during the lifetime of a major version?

@AdamPS

AFAICS we do need to worry about the version field and the special significance of the value -1.

Using the Drupal cache API would mean any full cache clear will definitely purge all of the cache entries - this is different to core's actual asset aggregation where we also need to worry about browser and edge caches we don't have control over.

If you want to support cache-clear-free deployments, you can use AssetGroupSetHashTrait::generateHash() - need to add it as a trait on an existing class of course. Since this API exists, I'm not sure why you're asking for another one, hence needs more info.

Even then, you didn't respond to the matter of maintainability. Would you be willing to make a commitment to the suggested code remaining correct and complete during the lifetime of a major version?

If you use actual core interfaces, then the code will be supported for a major release (give or take deprecations, but it'll still work even if deprecated). If you use side-effects or behaviour details of code, then it's not.

If you actually try to use the core APIs, and then are unable to do something, then it is valid to ask for one to be added, but instead of this the issue is mostly asking to add a replacement for what was an undocumented non-API, and we have enough API surface with multiple interfaces and multiple classes within the asset aggregation system already.

Thanks @catch.

Yes I already understand the point you clearly made several times - that Core was entitled to make this change according to the BC policy. In many similar cases I have seen a change record explaining that sites needed to change their code, and how to do so. In this case such a thing doesn't exist, so there is lack of clarity in how to proceed. If you prefer to close this issue then fine.

Personally speaking I am unable to dedicate any more time to working out the correct code. If there are sites with big newsletters then presumably they also have experienced developers who can do so, and hopefully share the results somehow.

I would appreciate any maintainer feedback on the following example please, and whether this is sufficient info to re-open the issue. Even the code we already have in Drupal Symfony Mailer (without caching) seems to depend on the internals of Drupal Core:

      if (($asset['type'] == 'file') && $asset['preprocess']) {
        // Optimize to process @import.
        $css .= $this->cssOptimizer->optimize($asset);
      }
      else {
        $css .= file_get_contents($asset['data']);
      }

The if test was required to avoid an exception. However there's no mention of this restriction on the function comment, which just says "Optimizes an asset." It seems that in future a minor Core version could change the details of which assets were optimised or not, for example (and I'm completely making this up obviously) it might decide to optimise external assets that were marked with a new flag static (guaranteed not to change). I suggest that if the asset cannot be optimised then this function should simply return the file contents. In this case, the optimised asset is equal to the original asset.

I also wonder if it's right that only file assets can be optimised. I realise that the CSS aggregation would only want to optimise file assets, but other code using the service might be different. If a site wanted to inline CSS that was external and contained @import that's exactly what it would need. Perhaps the result shouldn't be cached but that's a different matter - this function is about optimising not caching.

That exception has been thrown since 2013, you're right that the interface is missing @throws, a documentation issue to add that would be good, it's separate from adding new API methods here.

Removing the exception could be worth discussion, but that should be done in a separate issue too.

Thanks I raised #3392497: Remove or document restrictions on CssOptimizer::optimize()

Just a comment to say we are experiencing this (css/js assets not present when using aggregation) when using AMP contrib module.

I have already related the issue but I wanted to also let everyone here know that we have another failed case with a contrib module, maybe not so popular as symfony_mailer, but still 2.5k users might be affected by this (if they enable aggregation).

Hopefully the idea of adding a new method to be able to generate the aggregated files is reconsidered.

Please note that the patch attached in https://www.drupal.org/project/amp/issues/3374942#comment-15152404 tries to use the guzzle request approach (mentioned in https://www.drupal.org/project/symfony_mailer/issues/3371042) , but that's failing in too many scenarios:

• Local docker env
• Sites behind a proxy
• Sites requiring a cookie to grant access

Since we need amp for one of our client's site I'll be working on the related issue trying to add a workaround that ideally doesn't disable the aggregation for amp assets, but if we can not achieve that we will need to simply ignore aggregation in order to get amp pages working (even with a bad performance result).

I just got the following warning after upgrading a site from 9.5 to 10.2:

Warning: file_get_contents(themes/custom/my_theme/../../../../../modules/contrib/colorbox/styles/default/colorbox_style.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).
Warning: file_get_contents(themes/custom/my_theme/../../../../../modules/contrib/colorbox/styles/default/colorbox_style.css): Failed to open stream: No such file or directory in Drupal\system\Controller\CssAssetController->generateHash() (line 43 of core/lib/Drupal/Core/Asset/AssetGroupSetHashTrait.php).

I can fix that by patching the colorbox.libraries.yml file like this:

default:
  version: VERSION
  js:
    styles/default/colorbox_style.js: {}
  css:
    theme:
-      styles/default/colorbox_style.css: {}
+      styles/default/colorbox_style.css: { preprocess: file }
  dependencies:
    - colorbox/init

But that doesn't really solve the issue here. The Drupal site is located at /var/www/html/ (web root is at /var/www/html/web/) which means that the ../../../../../

resolves to /var/www/modules/contrib/colorbox/styles/default/colorbox_style.css
instead of /var/www/html/web/modules/contrib/colorbox/styles/default/colorbox_style.css
and of course that file doesn't exist. I'm not sure whether this (the ../../ too much) is a bug in core or in the colorbox module. Therefore I didn't create an issue for that yet.

Should this be moved back to NW or is more info needed?

From Drupal Symfony Mailer perspective, there have been no problems reported with the solution reached in the discussion here. Thank you to all those who helped.

Wanted to bump 1 more time before closing since this had a big group of users.

Based on #29 and the fact it's been almost 4 months since the last comment, I'm closing this for now. Feel free to reopen it if you don't agree.