Drupal 10.1.0 new aggregation breaks InlineCssEmailAdjuster

First we should find out, if anyone else is experiencing this and can confirm the issue and resolution.

Is there any better solution?

Thanks. It seems like a reasonable solution. It would be better if the fix could be done inside the module, e.g. in InlineCssEmailAdjuster to save every site from having to do to.

@AdamPS I agree, but first I think the docs should be updated, so people don't run into trouble? I did that now!
How should we proceed?

Similar issue for me on Drupal 10.1.1 but without using "email" library.
I'm using symfony_mailer_bc module on 1.2.1 and `symfony_mailer_bc/contact` attachment cause the same issue.

The suggested resolution applied to `symfony_mailer_bc/contact` also fix this issue.

Thanks - but please see #4.

It would be better if the fix could be done inside the module, e.g. in InlineCssEmailAdjuster to save every site from having to do to.

Facing the same issue.
Having to reset the file permissions, after clearing the cache.

I can confirm patch #7 does not solve the problem.

Thanks - but please see #4 and #9.

It would be better if the fix could be done inside the module, e.g. in InlineCssEmailAdjuster to save every site from having to do to.

This module is calling a standard Drupal function getCssAssets() and it's not working. Please can someone ask on Drupal Answers to get expert advice on the right solution. What is the right way to get the CSS file contents from a CSS library, including loading of all includes etc.? I don't believe that the right fix is to alter every library definition file to disable preprocess - we might even be reusing a standard library that we can't alter.

It feel like this is a Drupal Core bug - in a minor release there was a non-back-compatible change that has broken existing sites.

Facing same problem after project update to Drupal 10.1. I don't think set preprocess: false is a proper solution, because its possible, that email library will depend on another library which can be provided by a third-party module/theme and this will be a problem to adjust. Also, it makes no sense in that case to use libraries, because if we restrict dependencies - we're basically restricting usage of that, and it's not obvious behavior.

I have two proposal how we can solve it.

Solution #1

\Drupal\symfony_mailer\Plugin\EmailAdjuster\InlineCssEmailAdjuster::postRender in this method, the $this->assetResolver->getCssAssets($assets, TRUE) is called. The second parameter responsible for aggregation and optimization of CSS assets. By passing it as, TRUE it tries to return aggregated file path, but this file should be created by direct request since Drupal 10.1.

The solution is straightforward - pass FALSE instead. In that case, it returns original CSS assets which can be easily processed by file_get_contents($file['data']), because they are always exists as a part of the library. If it doesn't - it's not a module responsibility.

Basically, it is not important to use optimized CSS files by Inline CSS adjuster, because they are not part of email and size is not important. I think this is the simplest and risk-free solution.

Solution #2

This solution assumes that we stick with aggregated assets, in that case we should check the result file for existence first, and if it doesn't exist, well, trigger its creation by doing an HTTP request. It might sound a reasonable approach, but there are few concerns here. First of which - we should do a dedicated HTTP request, basically for no reason. This can create additional load and slow down the emails sending process. Secondly, this can lead to a problem when request is taking too long or even failed, in that case, the email won't be sent, or will be sent without styles (?).

The problem here, that the whole logic of building such file is in \Drupal\system\Controller\AssetControllerBase::deliver method. That means, we either should(not) copy-paste it to avoid HTTP requests, either doing HTTP request or fake it via direct call this method, which is also most like won't be expected to be used in that way.

Thanks @Niklan.

See #12/#13 I suggest the next step is either raise a bug against Drupal Core, or ask on Drupal Answers.

Solution #1

See this comment from InlineCssEmailAdjuster for explanation of why we need optimize. NB it should say @import

Request optimization so that the CssOptimizer performs essential processing such as @include.

Solution #2

As you say there are a few concerns😃. I'm not convinced this is the right answer.

Yes, you are right, without aggregation @import url('other.css') won't be included, did my testings, and it works with aggregation.

So, we need to focus on solution #2, but the problem is, it basically lacks of public API. But again, I don't think this is a core bug, this is how it works right now, it is a task of feature request. I'll create a separate issue for that.

So, actually, we have a public API for that. So, I've created a new patch which use CSS optimizers and grouper to build a CSS. This also process @import url('other.css') as expected.

The last submitted patch, 17: symfony_mailer-3371042-17.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Well, this patch (#17) will only work with Drupal 10.1+, even if we are going to use AssetCollectionGrouperInterface and AssetCollectionOptimizerInterface for dependency injection, the Drupal 10.0- doesn't have ::optimizeGroup() method.

Also, \Drupal\Core\Asset\CssCollectionOptimizer::optimize() have different signature for Drupal 10.0 and Drupal 10.1

10.1: public function optimize(array $assets, array $libraries);
10.0: public function optimize(array $assets);

Even if we are to address this, the Drupal 10.1 in that case will result in the same problem, it will prepare aggregated CSS filename, but won't generate it.

I think we have to do what #17 patch does, but provide a backward compatibility layer for 10.0- and remove it when 10.1 will become a minimum supported version by a module.

This patch basically handles CSS differently for Drupal 10.0- and 10.1+. Let's see how it goes.

Added tests that attach library with CSS file, which imports another CSS file via @import.

The tests-only patch expected to fail on Drupal 10.1 branch but pass on 10.0. It will show the exact problem for that issue.

The test with fix from #20 should pass on both branches.

Thanks @Niklan. That's a clever solution to the problem, perhaps the best we can do in the circumstances.

1) I still think it's a Drupal bug. 10.1 is a minor release and the release notes clearly state:

This minor release provides improvements and new functionality. It does not not break backward compatibility (BC) for public APIs.

However it did break BC for the public API - because code in this module that used to work now fails. Contrib modules should not be forced to test \Drupal::VERSION in this way. I would like to see comment/recommendation from a core maintainer/expert about this before committing here please.

2) It seems possible that the new code is significantly slower. In the old code I guess that the optimised CSS is written to file once then reused. In the new code I guess that the CSS is optimised again for each email. If you agree, we should also ask the core expert about this.

3) This patch breaks BC for code that has extended InlineCssEmailAdjuster. Probably we should allow the extra params to be NULL, and supply a default. We can have a deprecation notice for passing NULL.

4) Yes it would be great to add a test, thanks😃.

Sorry for noise, this commit conflicts with the patch https://git.drupalcode.org/project/symfony_mailer/-/commit/becf7869f4b65... I did the same :)

Update patched so they are applies correctly.

🎊 #24 results as expected. Without fix test failed on 10.1 but passed on 9.5. It failed with exactly the same problem:

file_get_contents(/vfs://root/sites/simpletest/20840169/files/css/css_HdH2dfVTlI0FmVLOZ7NJXm6Cx2rIsHeSvnEihBvtkrk.css?delta=0&language=en&theme=core&include=eJwrrsxNy8-rjM9NzMxJLYovSS0u0c_My8nMS9VLy88HAMGgDC4): Failed to open stream: No such file or directory

And second patch shows that it fixed now on 10.1 and still works as expected on 10.0.

This patch breaks BC for code that has extended InlineCssEmailAdjuster. Probably we should allow the extra params to be NULL, and supply a default. We can have a deprecation notice for passing NULL.

It's not clear for me in which way it breaks it? If they call parrent::postRender() it should work the same.

Thanks

It's not clear for me in which way it breaks it? If they call parrent::postRender() it should work the same.

The problem is if they override the constructor and call parent::__construct() - they will not pass enough arguments.

My opinion is that such plugins should be declared final or internal, don't see why anyone should extend it and module maintainers care about that BC for no reason. (it's better to create abstract class to extend, but module implementation should be final anyway) But since it is already public, I can only propose to remove these new dependencies from the constructor and trigger deprecation warning when it called that way and inject them from global container.

  public function __construct(array $configuration, $plugin_id, $plugin_definition, AssetResolverInterface $asset_resolver, ?AssetCollectionGrouperInterface $cssCollectionGrouper = NULL, ?AssetCollectionOptimizerInterface $cssCollectionOptimizer = NULL) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);
    $this->assetResolver = $asset_resolver;
    $this->cssInliner = new CssToInlineStyles();

    if (!$cssCollectionGrouper) {
      @trigger_error('Calling InlineCssEmailAdjuster::__construct without the $cssCollectionGrouper argument is deprecated in drupal/symfony_mailer:1.2.2 and will be required before drupal/symfony_mailer:2.0.0. See https://www.drupal.org/project/symfony_mailer/issues/3371042.', E_USER_DEPRECATED);
      $cssCollectionGrouper = \Drupal::service('asset.css.collection_grouper');
    }
    $this->cssGrouper = $cssCollectionGrouper;

    if (!$cssCollectionOptimizer) {
      @trigger_error('Calling InlineCssEmailAdjuster::__construct without the $cssCollectionOptimizer argument is deprecated in drupal/symfony_mailer:1.2.2 and will be required before drupal/symfony_mailer:2.0.0. See https://www.drupal.org/project/symfony_mailer/issues/3371042.', E_USER_DEPRECATED);
      $cssCollectionOptimizer = \Drupal::service('asset.css.collection_optimizer');
    }
    $this->cssOptimizer = $cssCollectionOptimizer;
  }


  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    return new static(
      $configuration,
      $plugin_id,
      $plugin_definition,
      $container->get('asset.resolver'),
    );
  }

Intentionally removed comments for this comment to make it smaller.

There are other ways around to solve that issue, but all of them require minimal involvement from the developer who extended this plugin.

don't see why anyone should extend it and module maintainers care about that BC for no reason

Yes it's a fair point, because it's a bit tedious otherwise. Probably even I had allowed adding to constructors in previous commits. So I'm willing to ignore this one - this isn't Drupal Core, and we don't need such high BC standards.

Look more closely at my feelings, I believe that I was raising a low importance comment there because I was feeling pressed, apologies for that. Since #12 I asked repeatedly for this matter to be raised properly with Drupal Core experts and apparently been ignored. So I'll aim to be clear on this point now😃. This issue is postponed waiting for a response from the Drupal Core maintainers regarding the fact they have apparently made a non-BC change in a minor release. I'm not going to follow that up myself right now, because I'm still working on migrating my sites from D9😃. Please can anyone who is interested in resolving this issue raise the matter in the appropriate way?

I have reopened Core issue 📌 Add an API method to get the contents of a CSS or JavaScript aggregate from a URL Closed: works as designed and added a comment linking here. This seems a good place to start the investigation.

Needs work based on comments 1&2 from #22. I mentioned these same points on the core issue.

However it did break BC for the public API - because code in this module that used to work now fails.

Not quite. AssetResolver::getCssAssets() is returning the same thing (or at least equivalent things), the issue is that the behaviour of the function has changed (i.e. it doesn't also create the assets on disk now). However that's an internal implementation detail of the class, not part of the public API. https://www.drupal.org/about/core/policies/core-change-policies/bc-policy → has a long list of what is and isn't considered public API.

I think the version check for now, and adding an API method to core which forces on-disk aggregate creation are both good ideas fwiw.

+++ b/src/Plugin/EmailAdjuster/InlineCssEmailAdjuster.php
@@ -73,9 +97,13 @@ class InlineCssEmailAdjuster extends EmailAdjusterBase implements ContainerFacto
     $assets = (new AttachedAssets())->setLibraries($email->getLibraries());
-    $css = '';
-    foreach ($this->assetResolver->getCssAssets($assets, TRUE) as $file) {
-      $css .= file_get_contents($file['data']);
+    $assets = $this->assetResolver->getCssAssets($assets, FALSE);
+
+    if (version_compare(\Drupal::VERSION, '10.1', '>=')) {
+      $css = $this->prepareCss($assets);
+    }
+    else {
+      $css = $this->prepareLegacyCss($assets);
     }

Could probably check file_exists() and use the old behaviour as long as the aggregate exists. This would mitigate some of the performance impact.

Also could potentially make a guzzle request to the URL if the file exists in lieue of a method to create a file manually.

Could probably check file_exists() and use the old behaviour as long as the aggregate exists. This would mitigate some of the performance impact.

We considered that approach, but the thing is, in most cases (99% as far I can see), it will never exist, because email will probably use their own sets of assets (CSS), rather than main ones. E.g. symfony_mailer/email — it will never will be a part of the main requests, hence, will never be generated.

Also could potentially make a guzzle request to the URL if the file exists in lieue of a method to create a file manually.

Yeah, we also discussed that, but it sounds weird and can lead to other problems. We simply bootstrap Drupal for the task that can be done in the same process — it is wasting time and resources, not to mention that any HTTP problem during request will interrupt email sending. To create an aggregated file, we have to either doing HTTP request (or fake it and rely on other API), either create it manually using other available API. It does not sound very reliable solution. So basically, we have an API to get the result file URI, we have an API to get aggregated file contents, but we have no API for saving this file properly to an assets stream wrapper. We basically have to re-implement part of the logic inside, \Drupal\system\Controller\AssetControllerBase::deliver() which sounds like a lack of API. Even if we're going to implement the manual saving of the file to expected URI if files are not created yet, should we take care about validation and state management which is done inside AssetControllerBase::deliver()?

Even if we're going to implement the manual saving of the file to expected URI if files are not created yet, should we take care about validation and state management which is done inside AssetControllerBase::deliver()?

The state management is vestigial - that's going to be entirely removed in 10.2.x, so not necessary.

The validation could be skipped - it's to avoid URL manipulation/disk filling.

Agreed we should add an API for it though.

Could we do it like this...?

1. Create a Core patch with the API.
2. Copy the same code into this module temporarily. We could use a hook to swap the implementation of the affected service for the patched version.

@AdamPS it would be possible to subclass the core Asset Resolver, add the method, and then implement a service modifier to swap out the implementation. However, I think just a method_exists() check and falling back to a copied version of the logic would probably be less to maintain - i.e. then you don't need to worry if another module is also trying to swap out AssetResolver and it would also be easier to remove once you require >=10.2

Thanks @catch.

So as I understand it the code will look something like this.

1. In InlineCssEmailAdjuster::postRender(), prior to calling file_get_contents() we check file_exists().
2. If this fails, call a new function $this->buildCssAsset(), which contains the copied logic. We don't need to check \Drupal::VERSION because the file will always exist in older versions so this function will never be called.
3. Once core versions without the API are no longer supported, we can remove $this->buildCssAsset() and call the core function directly.

The above patches still had problems for me, but I needed to get this somehow running for a client-project. I created this simple patch based on #11. This is not to be seen as a real solution, only as a temporary fix until we have one. Just sharing this if anybody else is in a bind, and just needs this running.

I've written up a bit more of a plan on 📌 Add an API method to get the contents of a CSS or JavaScript aggregate from a URL Closed: works as designed .

Great thanks

I started looking at 📌 Add an API method to get the contents of a CSS or JavaScript aggregate from a URL Closed: works as designed , but once you get past all the URL parsing (because we're having to encode information in a URL for the controller to use), any helper method we could add would either be too much tied to the URL format or too low level. You'd either have to mock a Request object and pass it in, or it would just be a wrapper around existing API methods that we already have and require loads of boilerplate to actually use.

I then came back and re-read this issue including the patch in #3371042-24: Drupal 10.1.0 new aggregation breaks InlineCssEmailAdjuster → and I think it's the approach that should have been used in the first place - i.e. use core's existing API to take some libraries and get CSS contents out of them.

The libraries being requested here are not the same as ones that would result from any request to Drupal pages, so there is no duplication of the aggregate. Also there's no need to have an aggregate file because it's only the contents (the actual CSS) that is needed in the end.

That leaves the performance concern, and I think that can be handled by using Drupal's regular cache API to cache the resulting CSS string.

I've marked the other issue by design and would recommend continuing here.

Also I think it would be worth checking if the ::prepareCssAssets() method would work on Drupal 10.0 and lower (to me it looks like it should). If that's the case, once caching is added, there'd be no need for a core version check then.

Thanks @catch. It could be useful to other developers to document these required code changes in the change record. AFAIK it applies to swiftmailer at least, if anyone is still maintaining it.

It still seems to me that we've lost something useful from Core: get the CSS from a set of assets, with caching. Anyway I raised 📌 Add caching of CSS Active for the caching, and I guess other similar modules can do their own thing too.

The Core version check can be removed come D10.0 EOL in December in any case, if not before. I raised 📌 Remove Core version check once D10.0 is unsupported Closed: outdated for that.

So let's get this one in.

For the constructor change, the first block of code in #27 looks good, except we need to keep the extra lines in create() else we trigger our own deprecation warning. Or we could just ignore the BC problem, and keep the code as is, which is tempting (otherwise so much pain simply to use a new service!) but it could cause a problem for someone.

Otherwise it looks really good except I have some comments on the test.
1) We can put the test into the existing SymfonyMailerKernelTest to save duplication.

2) We should use the normal email sending:
$this->emailFactory->newTypedEmail()->addLibrary()->send()
then
$this->assertBodyContains()

I had a quick look at the source code. My understanding is that, ultimately we just need the CSS code from the library and then convert them into inline CSS embed into the HTML body of the email.

The code to convert the inline CSS are https://git.drupalcode.org/project/symfony_mailer/-/blob/1.3.1/src/Plugi...

 if ($css) {
      $email->setHtmlBody($this->cssInliner->convert($email->getHtmlBody(), $css));
    }

Why don't we just load the original CSS file (non-optimized) to get the content?

So change the line at line 77 https://git.drupalcode.org/project/symfony_mailer/-/blob/1.3.1/src/Plugi...
from

foreach ($this->assetResolver->getCssAssets($assets, TRUE) as $file) {

to

 foreach ($this->assetResolver->getCssAssets($assets, FALSE) as $file) {

Performance-wise, the CSS code will be converted to inline CSS, at the end, there is no different between loading from the original CSS file and loading from a optimized CSS file.

The patch.

Thanks for the idea however optimize is needed for @import, please see #15 and #16. Patch #24 has some good tests for this that will presumably fail with your patch.

Thanks @Adam.

Yes, you are right. I used the patch #24 to test my patch locally. It did fail as the CSS @import rule is not working.

I will stick with the solution 1 to see if there is anything I can do to get it works as I believe that it is a straightforward and less dependencies to core.

The patch from #24 should work without any dependencies on Core changes. It just needs some minor changes before commit, described in #45.

I would suggest the following changes for patch #24,

• Use CssOptimizer::loadFile() instead of CssCollectionOptimizerLazy::optimizeGroup () or CssCollectionOptimizer::optimize().
  The \Drupal\Core\Asset\CssOptimizer is available for all Drupal versions back to 8.9. See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Asset%21C...
  So there is no compatible issue and no need to check the Drupal core version.
• Cache the CSS content via Drupal cache API.
  Previously, I thought CssCollectionOptimizerLazy would cache or store the CSS content in a static file for a better performance. After looking into it, I realised that it is not the case. So there is a performance glitch in which we repeat the same thing for every email, which is to load all CSS files and optimise them. We need to cache the CSS content for a better performance.

The difference between patch #24 and #54.

I think an aggregated file should be cached on disk, not in database. We should avoid loading database in this case.

Since the aggregated result is exactly the same as it will be for direct request for regular pages, we better to save it under expected filename in asset://css. If someone shares libraries for main theme and email, this will be served for both from the disk.

Basically, I highly against overload database for such things. It actually can be much slower than runtime preparation when website under heavy load and DB is busy.

According to the source code from Core https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/lib/Drupal/...

I can't see anywhere those content would be cached on disk (file).

Drupal cache all page contents that have been ever accessed in the database.
The cache.render bin is designed for caching large contents, such as html markup for entire page across the website.

I can't see anywhere those content would be cached on disk (file).

Yes, because this logic is moved to \Drupal\system\Controller\AssetControllerBase::deliver() + \Drupal\Core\Asset\AssetDumperUriInterface::dumpToUri() in 10.1

Drupal cache all page contents that have been ever accessed in the database.
The cache.render bin is designed for caching large contents, such as html markup for entire page across the website.

Database in most cases for big Drupal websites is a bottleneck, and introducing additional I/O during email send, which can be in thousands in minute in bulks - waste of resources and slowing database further. If an email has 10 assets, it will create a 10 DB requests per email at minimum.

Also, this is not a cache.render responsibility to store CSS, it is intended to store rendered HTML markup from render array. At least it should be using cache.data bin. If you want to store it in cache, I think it is better to introduce a dedicated bin for that (e.g. cache.symfony_mailer.css), in that case, developers will have control over CSS saved in this bin. For example, I can switch it to cache.backend.null, so each email will generate CSS in runtime every time it sends, or something like cache.backend.php to save it on disk and avoid database usage. At least it can be controlled.

But I still think, if we are going to «cache» result, it should mimic the core behavior on 10.1+ and use asset dumper for that.

#54 looks good to me.

@Niklan The cache API is absolutely fine for storing long strings in including CSS and it's not necessarily slower than reading a file from disk. i.e. the cache could be in redis or memcache, whereas the file system could be a networked filesystem, or on S3 etc.

If the CSS was being served to a browser it would be a bad idea due to lack of http caching etc., but it's sent inline in the e-mail.

+++ b/src/Plugin/EmailAdjuster/InlineCssEmailAdjuster.php
@@ -73,14 +97,47 @@ class InlineCssEmailAdjuster extends EmailAdjusterBase implements ContainerFacto
+      // Cache ID.
+      $cid = 'symfony_mailer:file:' . $file_name;
+      // Load the cached CSS content for this library.

One issue here - I think this would need to add the css_js_query_string so that if css/js is invalidated, the cache items are too. Because it's in the cache API, drush cr will clear it for other cache clears/deployments anyway so it's probably fine otherwise.

Thanks @Nikita for looking at the #54 patch and your advices.

Drupal Internal Page Cache module and Dynamic Page Cache module from Core does cache huge string (HTML including styling) in the database.

Agree Database I/O could be a bottleneck of performance for a high traffic website. But quite frankly, we are discussing the CSS for an email template, given that I don't know in what case we have to deal with huge CSS files that would make any difference performance-wise.

I did test #24patch in one of my develop environment by sending a test Email via UI (admin/config/system/mailer/test) and a contact form email by Contact form module. Then I searched the key word of '.text-small' for the test Email and '.contact-email-intro' for the contact form Email in the CSS caching folder (/sites/default/files/css/). I couldn't find those keywords in any cached CSS file. That is just a quick test, I might miss something. If we could have more eyes on it to double check, we would be more confident about it.

I think it is better to introduce a dedicated bin for that

I think this is a good idea. I am with you.

I think it safer to wrap $file_name in some sort of md5() to make sure that string will be more predictable

Totally agree.

By the way, I only tested patch #24 as an authenticate user. I didn't test it as an anonymous.
But there is no aggregated (optimised) CSS files generated for the Email in my case.

@Mingsong you are right, #24 doesn't cache CSS in any way for 10.1+ workaround (for 10.0- Drupal handles it itself). So it is always generated in runtime for each email (which is also can be bottleneck). It just doesn't sound right to me to save it into cache.render, because, if I want to override it for some reason, it will affect the whole render cache for no reason. I'll prefer to have control over it.

Thank you, @catch, for your input regarding Cache API usage in that case.

Thanks @Niklan,

I make a minor change to patch #54 to use the MD5 encryption for the file name as suggested.

And some code standard correction.

Here is the new patch.

Thanks @Nathaniel for looking at the patch #54.

One issue here - I think this would need to add the css_js_query_string so that if css/js is invalidated, the cache items are too. Because it's in the cache API, drush cr will clear it for other cache clears/deployments anyway so it's probably fine otherwise.

Since we are using the cache for an email not a HTML page rendering in a browser, and the CSS will be converted into the inline styling, that means the CSS files won't be included in the HTML body of an Email, we don't need the CSS query string. Is my understanding correct?

Difference between patch #54 and #65.

better to beware md5(), use crc32b instead as http://drupal.org/node/845876

Great thanks everyone. Here are my comments:

1) We no longer need the prepareCss() function, so let's keep it in preRender(). Then please move the comment explaining the optimization down to before the call to cssOptimizer().

2) From #61:

One issue here - I think this would need to add the css_js_query_string so that if css/js is invalidated, the cache items are too. Because it's in the cache API, drush cr will clear it for other cache clears/deployments anyway so it's probably fine otherwise.

I think this may mean to call \Drupal::service('asset.query_string')->get(); and include the value in the cache ID.

3) + $cid = 'symfony_mailer:file:' . $file_name;
This module might later do other caching. Let's make it clear this cache related to CSS. How about symfony_mailer:css: or symfony_mailer:cssfile:?

4) The comments from #45 still need doing please.

+++ b/src/Plugin/EmailAdjuster/InlineCssEmailAdjuster.php
@@ -47,11 +63,17 @@ class InlineCssEmailAdjuster extends EmailAdjusterBase implements ContainerFacto
-  public function __construct(array $configuration, $plugin_id, $plugin_definition, AssetResolverInterface $asset_resolver) {

Please don't commit this without providing backwards compatibility, we've already been bit (multiple times) by this module making breaking changes in minor versions.

This particular case may not be the most likely class to have been subclassed by users, but you can pretty much guarantee someone has done it. Sometimes you can guard a bit against constructor changes by only overriding the create() method in your subclass and tack on the extra dependencies there, but it's no longer true DI and makes it harder to test since now you need tests to create the DI container and register the dependencies there instead of just passing them to the constructor.

At least let the new arguments default to NULL, fill in from \Drupal::service() if not set, and trigger a deprecation warning instead of just expecting them to be set.

Responding to #68

4) The comments from #45 still need doing please.

I created a test-only patch for the following changes:

1) We can put the test into the existing SymfonyMailerKernelTest to save duplication.

The InlineCssEmailAdjusterTest is remvoed.

2) We should use the normal email sending

Created a new test funciton SymfonyMailerKernelTest::testInlineCss(), which is

  /**
   * Inline CSS adjuster test.
   */
  public function testInlineCss() {
    $to = 'to@example.com';
    // Test an email including the test library.
    $this->emailFactory->newTypedEmail('symfony_mailer', 'test', $to)->addLibrary('symfony_mailer_test/inline_css_test')->send();
    $this->readMail();
    $this->assertNoError();
    // The inline CSS from inline.text-small.css should appear.
    $this->assertBodyContains('<h4 class="text-small" style="padding-top: 3px; padding-bottom: 3px; text-align: center; background-color: #0678be; color: white; font-size: smaller; font-weight: bold;">');
    // The imported CSS from inline.day.css should appear.
    $this->assertBodyContains('<span class="day" style="font-style: italic;">');
  }

I think this may mean to call \Drupal::service('asset.query_string')->get(); and include the value in the cache ID.

The main purpose of using Drupal asset query string is to avoid out of date browser-caching, which we don't need to worry about for CSS files loading from the local file system rather than a remote URL cached by a browser.

By the way, the 'asset.query_string' service won't be available until Drupal 11.

See https://www.drupal.org/project/drupal/issues/3358336 📌 Deprecate _drupal_flush_css_js() and create a new AssetQueryString cache service Fixed

Regarding the concern on md5 from #67

better to beware md5(), use crc32b instead as http://drupal.org/node/845876

As we are not using md5 to encrypt any secret here other than using it to avoid a potential SQL injection attack from a file name.
Imaging if the CSS file name is something like "1'); DROP TABLE STUDENTS; --", I don't know what would happen if this kind of file name was includeded in the cache ID. Would Drupal core take care of it?
Also, md5 will make sure the length of the file name (including the absolute path) won't be too long for a cache ID.

I have the same error message as in the description on my local instance and the following one on our test server (which configured exactly the same as the future production server except the hostname):

Warning: file_get_contents(): open_basedir retsriction in effect. File(/sites/default/files/css/css_OTs7jT_btKWU2MLco9kpWeRL-rV2o_TgZa4ICtteRCk.css?delta=0&language=de&theme=drowl_child&include=eJxLKcovz4lPzsjMSdFPzU3MzNEprsxNy8-rjAdxUov0S1KLSwAYzA7M) is not within the allowed path(s): (/var/www/vhosts/***Hostname removed***/:/tmp/:/var/lib/php/sessions) in Drupal\symfony_mailer\Plugin\EmailAdjuster\InlineCssEmailAdjuster->postRender() (line 78 of modules/contrib/symfony_mailer/src/Plugin/EmailAdjuster/InlineCssEmailAdjuster.php).

That error message ("open_basedir restriction in effect") might give a better understanding of what's causing the error than "Failed to open stream: No such file or directory": The path to the CSS file starts with a / so it is kind of rooted. If we check other usages of the standard Drupal function getCssAssets() we i.e. can find Drupal\Core\Render\HtmlResponseAttachmentsProcessor::processAssetLibraries() which passes the result of getCssAssets() directly to Drupal\Core\Asset\CssCollectionRenderer::render() to create HTML <link> render arrays for all assets. This means, in that (and all other cases I found) the path is web-rooted to be output as

<link rel="/sites/default/files/css/css_OTs7jT_btKWU2MLco9kpWeRL-rV2o_TgZa4ICtteRCk.css?delta=0&language=de&theme=drowl_child&include=eJxLKcovz4lPzsjMSdFPzU3MzNEprsxNy8-rjAdxUov0S1KLSwAYzA7M" />

but in our case the path is rooted against the linux file system. Therefore, I don't understand the very long discussion and complex ideas to solve this. In my opinion it is very easy to solve:

Proposed resolution

• Prepend $file['data'] with Drupal's app.root path value. Considering the differences between >= 10.1 and < 10.1 we can do this conditionally only if stripos($filePath, '/') === 0 // Make sure to use type-safe comparison as we want to check against position 0 and not FALSE indicating that $haystack does not contain $needle!
• Furthermore, we need to load the CSS file from the file system and not via HTTP(S) for inlining, so we need to get rid of the url params.

Starting example:

public function postRender(EmailInterface $email) {
    // Inline CSS. Request optimization so that the CssOptimizer performs
    // essential processing such as @import.
    $assets = (new AttachedAssets())->setLibraries($email->getLibraries());
    $css = '';
    $rootPath = \Drupal::getContainer()->getParameter('app.root');
    $logger = \Drupal::logger('symfony_mailer');
    foreach ($this->assetResolver->getCssAssets($assets, TRUE) as $file) {
      $filePath = $file['data'];

      // Only if we have a rooted path (which means it is web-rooted).
      if (stripos($filePath, '/') === 0) {
        // Correct the path to be linux-rooted.
        $filePath = $rootPath . $filePath;
        // Remove the query string which we don't need to load the file.
        if (($pos = stripos($filePath, '?')) !== FALSE) {
          $filePath = substr($filePath, 0, $pos);
        }
      }
      
      try {
        $logger->info('Loading CSS from file: %file.', ['$file' => $filePath, 'file' => $file]);
        $css .= file_get_contents($filePath);
      }
      catch (\Exception $e) {
        $logger->error('Failed to load CSS from file: %file. Error: %error', ['$file' => $filePath, 'file' => $file, '%error' => $e->getMessage(), 'error' => $e]);
      }
    }

    if ($css) {
      $email->setHtmlBody($this->cssInliner->convert($email->getHtmlBody(), $css));
    }
  }

I just created a separate branch and MR 60which so far works, but we need to add the changes described in #38 there as well.

Does the solution from #72 work for Windows server?
Also, does it introduce a performance issue?

Solution from #72 failed in D10.1 test.

Exception: Warning: file_get_contents(/var/www/html/subdirectory/sites/simpletest/84609511/files/css/css_hJi4t2sszZP_5G0a_zmoJM2K5l53gB6zEjvJD8VJetM.css): Failed to open stream: No such file or directory
Drupal\symfony_mailer\Plugin\EmailAdjuster\InlineCssEmailAdjuster->postRender()() (Line: 112)

I assume that it because after D10.1 the CSS file does not exist at all. Not a path issue but a non-exist file issue.

Thanks @Mingsong the test changes look good to me, so I committed them. The previous patch now needs a reroll. Also it would be good to have a summary of which comments have not yet been done.

The main purpose of using Drupal asset query string is to avoid out of date browser-caching, which we don't need to worry about for CSS files loading from the local file system rather than a remote URL cached by a browser.

I propose that:

1) We need to clear our cache in response to `drush cc css-js`.

2) We need to ensure our cache is not stale when the CSS files change on disk. At least on a dev site without aggregation enabled I would expect a changed CSS file to be used automatically without having to force a cache clear.

By the way, the 'asset.query_string' service won't be available until Drupal 11.

True so we can do it the old way - check the state key directly.

As we are not using md5 to encrypt any secret here

I don't see that we need to protect against hostile CSS filenames - only a developer can choose these, and the developer can do what they like anyway. We need to avoid clashes with different files generating the same hash and to handle very long filenames.

The Drupal Core policy is very clear, so I don't think we should go against it here.

For Drupal 7 and later core and contributed modules, the md5() and sha1() hash functions should never be used in any code, since they are considered obsolete and potentially insecure for some applications. This is a settled policy for Drupal core.

Maybe we can do whatever core does for generating names of aggregates?

I assume that it because after D10.1 the CSS file does not exist at all. Not a path issue but a non-exist file issue.

Yes exactly. I think MR60 is the wrong direction unfortunately.

That leave the comments from #67,#68,#69 to do. Most/all of the remaining problems are with caching. Earlier I raised 📌 Add caching of CSS Active and I'm still willing to defer caching to that issue which might help get a first commit.

Please don't commit this without providing backwards compatibility, we've already been bit (multiple times) by this module making breaking changes in minor versions.

Core makes a strict BC promise, and yet it allowed a change that caused this issue and "broke" this module. This contrib module makes it's own less strict policy for BC. Especially in the earlier releases when the module was new some initial sub-optimal design choices needed to be corrected. Although you may not believe it, I have spent hours maybe even days writing code to help make upgrades easier for the people, and AFAIK it worked for most sites. In the end it's a balance - increasing BC saves time for sites, but takes time from developers. The developers are the ones who are putting the work in, so we can't afford to penalize them too much.

This issue is breaking compatibility with D10.1, so it's major and urgent. If I get a patch that works excluding BC on the constructor then I will commit it, with an explanation in the release note. If someone then creates another patch that adds BC then I would also commit that.

Related issue went in without change record so should be taken into account

we've already been bit (multiple times) by this module making breaking changes in minor versions.

I'm happy for constructive discussion on past BC breaks to help learn for the future - please use 📌 Seriously consider respecting semantic versioning Active . However so far the claims seem to be incorrect.

Let's keep this issue for it's intended purpose - a minor version of Drupal Core has broken this module😃.

It's becoming increasingly clear that correct caching is complex, and Core already contains lots of code for it. This module has exactly the same requirement for CSS caching as any other except that it needs the CSS immediately rather than on a subsequent HTML request. Therefore it doesn't make sense to me for this module to completely rewrite caching. Instead we need Core to provide direct access to the caching it already has.

Either

1. We use aggregates - which I agree we don't need, but they do exactly what we want including caching. This would need a Core function to trigger generating of an aggregate. This was rejected by @catch in 📌 Add an API method to get the contents of a CSS or JavaScript aggregate from a URL Closed: works as designed however it still seems like an option.
2. OR Core provides access to its caching such as drupal_css_cache_files.

Perhaps Core maintainers won't agree, but anyway, let's definitely leave caching out of this issue which is already complex enough. In most cases, email sending will not be a bottleneck as a single page generates one email. The main exception would be a newsletter with 10000+ subscribers.

#54 uses $this->cssOptimizer->loadFile which we shouldn't do as it is commented like this:

   * Note: the only reason this method is public is so color.module can call it;
   * it is not on the AssetOptimizerInterface, so future refactorings can make
   * it protected.

We can use optimize() instead, and we need to skip the call if 'preprocess' is FALSE or 'type' is not 'file'.

This issue is potentially disruptive - and some sites might even choose to stay on D10.0 to keep caching. I'm willing to create a new major version for it, as it seems like about the right time, with several other accumulated non-BC issues that we can also include. This would also solve the constructor BC problem mentioned in #69.

@AdamPS: Just wanted to say THANK YOU for the great work here and for the module in general. It's really impressive!

I'll go along with that :-)

Many thanks for the kind words😃.

Here is a reroll of #54.

Updated patch without caching and applying all remaining comments.

OK this is a nice simple/safe patch and I believe it fixes the bug. Please can other people test/review?

It probably does reduce performance for sending large batches of mails. As a reminder, this is caused by the removal of important function from Core: get CSS from assets efficiently by a means other than HTML. I will raise a new Core issue (or re-open the old one) requesting that the feature be brought back. I plan to document all this in the release notes and commit to a new minor branch 1.4.

(I changed my mind about a new major branch - if we did that then I would want to wait for any other non-BC fixes, which would delay this fix. Instead I made this patch non-BC by allowing NULL in the constructor.)

Patch #88 works for me.

Since I don't have an environment in which a old version of Symfony Mailer is installed, I am not able to test it for BC.

thank's AdamPS, last patch fixes inlinecss in my case, will report if there are issues about performance,(i am using sengrid with failover to amazon sending circa 5000 each day). using 1.3.1 with D10.1.3

Thanks @Adam so much for working on this module.

I think we still need to set those libraries as { preprocess: false }, right?

As the proposed resolution/MR(https://git.drupalcode.org/project/symfony_mailer/-/merge_requests/52/diffs) suggested.

OK this is a nice simple/safe patch and I believe it fixes the bug. Please can other people test/review?

The patch in #24 contains the test, why doing it manually? You can test with it on all supported Drupal versions by module. Most people coming here are more likely already on Drupal 10.1+, but it still should work on Drupal 10.0- after it released.

I think we still need to set those libraries as { preprocess: false }, right?

This was just a workaround and it not needed or recommended unless you want the effects: disable optimisations and @import. Testers please test without it.

why doing it manually?

Automatic testing has some benefits. Live testing and peer review are also important - that's what the status "RTBC" is for😃. Especially here when performance is affected.

The patch in #88 works for me.

OK, thanks everybody.

I'm still interested in any results from perf testing.

Can we get the project page updated with this issue being resolved? Right now it still says the project is incompatible with Drupal 10.1.

I think, this should not done before releasing a new version

I agree with both of the previous comments😃. Next steps are as in #89 - I realise it's important, and I will do them as soon as I have time.

I have created 1.4.0-beta1. I reopened 📌 Add an API method to get the contents of a CSS or JavaScript aggregate from a URL Closed: works as designed for further discussion of the removal of function from Core.

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

AdamPS → closed merge request !52

AdamPS → closed merge request !60

Thanks in Advance.

Using Drupal core 10.2.1 & symfony_mailer for email, mail is working fine but not in formatted way.