CommentFileSizeAuthor
#131 swiftmailer.css_inliner.2843327-131.patch7.89 KBAdamPS
#130 swiftmailer.css_inliner.2843327-130.patch7.89 KBAdamPS
#87 swiftmailer.css_inliner.2843327-87.8.x-2.x.patch10.37 KBTommyChris
#5 swiftmailer-cssinlinestyles-option-2843327-5.patch3.11 KBchristian-heiko
#8 swiftmailer-cssinlinestyles-option-2843327-8.patch3.63 KBdimr
#11 swiftmailer-add_css_inliner_support-2843327-10.patch6.82 KBandyg5000
#11 interdiff.txt6 KBandyg5000
#13 swiftmailer-add_css_inliner_support-2843327-11.patch7.52 KBandyg5000
#14 swiftmailer-add_css_inliner_support-2843327-14.patch6.96 KBzengenuity
#21 swiftmailer.css_inliner.2843327-20.patch6.99 KBAdamPS
#22 swiftmailer.css_inliner.2843327-21.patch4.9 KBAdamPS
#22 swiftmailer.css_inliner.2843327-interdiff-20-21.txt3.95 KBAdamPS
#31 swiftmailer.css_inliner.2843327-31.patch8.76 KBAdamPS
#31 swiftmailer.css_inliner.2843327-interdiff-20-31.txt6.39 KBAdamPS
#33 2843327-33.patch9.1 KBheddn
#33 interdiff_31-33.txt657 bytesheddn
#34 interdiff_33-34.txt5.64 KBheddn
#34 2843327-34.patch13.08 KBheddn
#49 swiftmailer.css_inliner.2843327-49.patch13.07 KBAdamPS
#51 swiftmailer.css_inliner.2843327-diff-34-49.txt530 bytesAdamPS
#57 swiftmailer.css_inliner.2843327-57.patch26.71 KBFMB
#57 swiftmailer.css_inliner.2843327-diff-49-57.txt626 bytesFMB
#58 swiftmailer.css_inliner.2843327-58.patch12.85 KBAdamPS
#58 swiftmailer.css_inliner.2843327-interdiff-57-58.txt2.08 KBAdamPS
#61 swiftmailer.css_inliner.2843327-61.patch10.38 KBAdamPS
#61 swiftmailer.css_inliner.2843327-interdiff-58-61.txt2.91 KBAdamPS
#64 swiftmailer.css_inliner.2843327-64.patch11.51 KBAdamPS
#64 swiftmailer.css_inliner.2843327-interdiff-61-64.txt1.74 KBAdamPS
#66 swiftmailer.css_inliner.2843327-66.patch10.15 KBStefdewa
#71 swiftmailer-css_inliner-2843327-71-D8.patch10.66 KBbohart
#71 swiftmailer-css_inliner-2843327-71-D8.interdiff.66-71.txt1.32 KBbohart
#80 swiftmailer-css_inliner-2843327-80-D8.patch10.7 KBpjbaert
#80 swiftmailer-css_inliner-2843327-80-D8.interdiff.75-80.txt2.62 KBpjbaert
#83 swiftmailer.css_inliner.2843327-interdiff-80-83.txt2.78 KBAdamPS
#83 swiftmailer.css_inliner.2843327-83.patch10.67 KBAdamPS
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

agoradesign created an issue. See original summary.

Lukas von Blarer’s picture

Yes, this would be a very useful feature.

FMB’s picture

As a quick note, I was unable to use Swiftmailer Css Inliner (the plugin seems to be registered, but not actually called), so I ended up overriding the SwiftMailer class this way:

namespace Drupal\my_project\Plugin\Mail;

use Drupal\swiftmailer\Plugin\Mail\SwiftMailer;
use TijsVerkoyen\CssToInlineStyles\CssToInlineStyles;

/**
 * Provides an alternative 'Swift Mailer' plugin to send emails.
 *
 * @Mail(
 *   id = "my_swiftmailer",
 *   label = @Translation("Swift Mailer (alternative)"),
 *   description = @Translation("Alternative Swift Mailter Plugin.")
 * )
 */
class MySwiftMailer extends SwiftMailer {

  /**
   * {@inheritdoc}
   */
  public function format(array $message) {
    $message = parent::format($message);

    // Force CSS to appear as inline "style" attributes.
    // @see https://www.drupal.org/node/2843327 for a more general approach
    $cssToInlineStyles = new CssToInlineStyles();
    $message['body'] = $cssToInlineStyles->convert($message['body']);

    return $message;
  }

}

Of course you'll have to configure Mail System so that it uses this new plugin instead of the original one; you'll also have to require tijsverkoyen/css-to-inline-styles with Composer.

Sorry for not being able to provide a proper and more general solution to this issue, though.

Lukas von Blarer’s picture

Great, I will try this on my current project and provide feedback.

I guess we would add an option in the Swiftmailer settings and only use the inliner if this is enabled.

christian-heiko’s picture

Status: Active » Needs review
FileSize
3.11 KB

this is a patch which gives the option to activate tijsverkoyen/css-to-inline-styles within the format of swiftmailer

Lukas von Blarer’s picture

@christian-heiko cool, I am going to test this soon on my current project.

FMB’s picture

Title: Support Swift Mailer plugins (especially CSS Inliner) » Support CSS inlining
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Updated this issue since we opted for a narrower scope. If you think that would be relevant, we could open a follow-up issue regarding the more general use of Swiftmailer plugins. #5 patch looks good and works correctly.

dimr’s picture

I have used the patch #5 and then I have realized that css file was missing in the CssToInlineStyles. I have create a new patch including the css file, I have presuppose that the file is mail.css and in under the default theme under the css folder. I have kept open the decision in case that that file doesn't exits creating a TODO with some comments.

dimr’s picture

Status: Reviewed & tested by the community » Needs review
andyg5000’s picture

Assigned: Unassigned » andyg5000
Status: Needs review » Needs work
andyg5000’s picture

Assigned: andyg5000 » Unassigned
Status: Needs work » Needs review
FileSize
6.82 KB
6 KB

@dimr, thanks for the initial work here. I made the following updates:

* Define the new "css_inline" config property in schema
* Added dependency injection for the mail manager and library parser
* Load the theme defined in mailsystem or default to the existing theme
* Parse libraries of theme
* Look for a "mail" library (should this be "swiftmail"? )
* Inline all css files defined in that libary

1) To use this, define a new library in your theme as such:

mail:
  version: VERSION
  css:
    theme:
      css/mail.css: {}

2) Edit mail system config and set the mail theme to the theme containing the library above

3) Enable the option in swiftmailer config

4) Send a test!

If people like this route, I'm happy to write the documentation and tests to hopefully get this merged.

Status: Needs review » Needs work

The last submitted patch, 11: swiftmailer-add_css_inliner_support-2843327-10.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andyg5000’s picture

zengenuity’s picture

This patch works for me, but it needs a re-roll to apply cleanly to dev.

PhilippVerpoort’s picture

I can confirm that adding the library as explained in #11 and applying the patch in #14 has worked for me.

osopolar’s picture

The patch in #14 applies well to current dev and works for me too.

The only thing which seems not to work was using composer to patch the module. The module gets patched, but composer was not installing the tijsverkoyen/css-to-inline-styles package. As I am new to that, I am not sure if I did something wrong. Anyway, that has nothing to do with the patch itself. I just had to do a composer require tijsverkoyen/css-to-inline-styles which works for now.

agoradesign’s picture

@osopolar no, you did nothing wrong. Composer itself can't take changed composer.json files in patches into account. Patches are applied after all dependency calculations and loading of modules have taken place.. so what you did (require the package in your project) was absolutely correct :)

Lukas von Blarer’s picture

If i remember correctly, running another composer update after applying the patch, fixes the issue.

yareckon’s picture

Couldn't we implement this as a separate module if we got an alter hook in the format() method? Issue here: #2949397: Add format alter as well as send alter

AdamPS’s picture

Nice patch. Comments:

  • Better perf and probably better accuracy to append all the CSS into one string then call CssToInlineStyles())->convert once.
  • Clearer to use $libraries['swiftmailer'] rather than $libraries['mail'] and reduce chance of clash with an existing library

#19 @yareckon It sounds like a good idea to have a format hook. But even so, why would we want to have a separate module for CSS inlining? It is an important feature and anyone who doesn't want it can disable it.

AdamPS’s picture

Status: Needs work » Needs review
FileSize
6.99 KB

Patch that fixes my own comments, plus I spotted that the form #description needed updating

AdamPS’s picture

Do we actually need the form setting css_inliner?

  • Why would someone create a library with this specific name then not want to use it? I propose that if the swiftmailer library on the theme exists then we should use it.
  • Why would someone want to have the CSS but not inline it? It won't work well, and anyway we don't have code for CSS that isn't inline.

Simpler patch that remove the form setting.

AdamPS’s picture

PS Does anyone know why the tests are failing? Could it be because of the extra composer dependency and the same problem as #16?

The last submitted patch, 21: swiftmailer.css_inliner.2843327-20.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 22: swiftmailer.css_inliner.2843327-21.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

FMB’s picture

Why would someone want to have the CSS but not inline it? It won't work well, and anyway we don't have code for CSS that isn't inline.

For debugging purposes, for instance. Mimemail has a CSS compressor module you can choose to enable or not.

AdamPS’s picture

  • #14+#21 Have a form setting to control whether to include CSS.
  • #22 Drops that because it seems redundant.

@FMB Please can you clarify what you would prefer?

  1. Would you like to go back to #21?
  2. Or would you like a different form setting to control whether to inline CSS? In that case, where can we put the not-inline CSS?
AdamPS’s picture

I should explain more clearly.

The patch in #14 has a setting 'css_inliner' with description:

The css of the rendered template (if set in template) will be parsed to inline styles to assure solid rendering in all email clients.

As far as I can see, it behaves like this:

  • If enabled, take extra CSS from a special library and add it inline.
  • If disabled, do nothing.

I think what you are expecting, and what the description suggests for is more like this:

  • If enabled, take any existing CSS and inline it
  • If disabled, leave the existing CSS as is

And I agree that sounds better. I think that "existing CSS" could come from the D8 standard method of attaching CSS libraries to the render array. I've not actually tried to code it yet. This approach avoids "magic names" and is flexible to allow modules to attach a different library for different mails.

Thoughts/comments??

FMB’s picture

AdamPS: let's say you have CSS in the <header> of a mail template. I think it should work that way: if you don't enable "inlining" this CSS will be applied (only by a few email clients); however, if you enable it, HTML tags will be rendered with inline CSS.

AdamPS’s picture

@FMB OK, thanks for the clarification.

The complication is that the patch in this issue (#14 onwards) actually does two things:

  1. Fetch some extra CSS from a theme library, which is is a really useful new feature. The existing method of static CSS in the template is not so flexible and a template is not really the "Drupal right place" to do styling.
  2. Inline CSS

I think you are asking for a config option that controls (2).

At the moment the config option controls (1)&(2): when you turn it off, the extra CSS is lost, which is not likely to be what someone wants, and is not what the config option description says would happen.

If we want "extra CSS from library" and "option for inline CSS" then I think we would need to put the extra CSS into the HTML if "inline CSS" is off. I have some code roughly working that does this, but it is a bit complicated and messy - e.g. it introduces a new template variable and everyone would have to add {{css}} to their templates, but then it won't be used 99% of the time if inline is on.

So it makes me wonder how important is it to be able to have CSS not inline? I don't think the popular module "Mime Mail" has an option for that, and as you said most email clients don't support it.

AdamPS’s picture

OK, I guess we could do this??

The config option is now called "Inline CSS" and it does exactly what you would expect.

Main changes from #14

  1. Can add arbitrary libraries - keeping "$mail_theme/swiftmailer" as the default.
  2. Use core services 'asset.resolver' and 'file_system' rather than making our own code because they handle lots of extra cases (library dependencies/ordering, CSS files on public:// etc).
  3. If not "Inline CSS" put the extra CSS into the <head>.

Status: Needs review » Needs work

The last submitted patch, 31: swiftmailer.css_inliner.2843327-31.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

heddn’s picture

FileSize
5.64 KB
13.08 KB

It did. Here's some code standard fixes. From what I saw of the code, this looked like in pretty good shape.
I hope that doesn't mean I can RTBC this.

I'm going to see how tests fair with the CS fixes, but I'll probably just flip this fine specimen of work over into the RTBC queue shortly.

heddn’s picture

No more CS failures. All tests are passing. Do we want/need tests about inlining the CSS?

AdamPS’s picture

@heddn Good work. I have reviewed all your changes and they look fine to me. I think that means we are RTBC because whatever work one of us did the other has reviewed and tested.

However you are right, it would be a good idea to have some tests.

pmoncel’s picture

Patch is OK for me : css inlines are well pushed in the html code and remain in the head of the message.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

So, let's go to RTBC then. We've had some reviews. The code works, but no one has said we need tests definitively. Let's leave that to the committer to decide and push back as needed. Hopefully this can just land.

Anybody’s picture

Great! When will we see a new dev release containing this? Confirming RTBC :)

Anybody’s picture

I just found a little problem for composer installs: "tijsverkoyen/css-to-inline-styles": "^2.2" seems to be unknown because it's no composer repository and the repository / package is not defined. What can we do?

Even if I use composer install in the swiftmailer folder it doesn't work.

agoradesign’s picture

Strange, because the project is definitely registered on packagist.org: https://packagist.org/packages/tijsverkoyen/css-to-inline-styles

AdamPS’s picture

@Anybody patches that changes the composer.json are tricky because composer reads the file from the repository rather than the local file - see comments #16-18. Composer install in the swiftmailer folder won't help because it would just download the file to the wrong directory and class loader.

Anybody’s picture

Sorry @agoradesign,

@AdamPS is right and you are right too. It was my mistake / the problem described in #42. I now required tijsverkoyen/css-to-inline-styles manually now. The patch is OK and things will be allright once the patch becomes part of the new release. Sorry for the trouble and thank you for the quick response. Still RTBC and hoping for a new dev release containing this great improvement!

agoradesign’s picture

no problem. there was no trouble at all for me. :)

I haven't looked into the patch at all, otherwise I could have told you the same as Adam. I'm subscribed to the issue and was just wondering, why there would be a package included that won't exist

AdamPS’s picture

Priority: Normal » Major
Parent issue: » #2979443: [META] Roadmap for stable

I propose this as a key issue for the next release as newsletters don't really work without it.

Anybody’s picture

Yes that's perfect. At least for stone-age mail programs like outlook ;)

Anybody’s picture

Confirming RTBC after using the patch for weeks on several sites now. Can we get this into the dev next release / stable?

Anybody’s picture

Any active maintainer in this issue who could create a new release with this important patch? If there are no active maintainers I'm willing to offer my help.

AdamPS’s picture

FMB’s picture

AdamPS: could you provide an interdiff? Makes reviews easier, thanks.

AdamPS’s picture

Unfortunately my diff tools reject patch #34 for some reason, so seeing as it was just a one line change I edited the patch by hand. That means my interdiff won't run either, but I have posted the output from diff.

FMB’s picture

Status: Needs review » Reviewed & tested by the community

Tested last patch on a clean Drupal 8 install, looks good to me, thanks.

Anybody’s picture

Status: Reviewed & tested by the community » Needs review

Retesting #49 against Core 8.7.x and 8.6.x. Is there an active maintainer for this project who can commit this afterwards?

heddn’s picture

Status: Needs review » Needs work

Looks like tests are failing?

Anybody’s picture

@heddn, please test #49 manually. To me it looks like failing tests are unrelated.

Package operations: 8 installs, 13 updates, 0 removals
- Updating sebastian/version (1.0.6 => 2.0.1): As there is no 'unzip' command installed zip files are being unpacked using the PHP zip extension.
This may cause invalid reports of corrupted archives. Installing 'unzip' may remediate them.

I guess the testbot can't handle the dependency:

"tijsverkoyen/css-to-inline-styles": "^2.2"

We're using the patch since month in our composer.json patches (automatically) without any problems.

FMB’s picture

There seem to be some minor coding standards issues, though.

FMB’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
26.71 KB
626 bytes

Really minor indeed! I hope Anybody's explanations suit you ^^

AdamPS’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
12.85 KB
2.08 KB

I installed this patch on a new site, which meant I accidentally tested without css_inliner and hit some problems.

1) Warning on upgrade to new version

Undefined index: css_inliner

We need to fix that with an update hook.

2)

Error: Call to a member function appendChild()

This is because $dom->loadHTML doesn't support HTML5 which Drupal 8 uses extensively. I think that means we may have to reject the idea of taking CSS from libraries when not inline CSS. The motivation for this was discussed in #26-#29 and is not mainline, but just for debugging.

This means that if css_inliner is false, any CSS from libraries is discarded. So once more I think it might be better not to have a form setting at all and instead inline CSS if and only if there is CSS from libraries. However I haven't heard anyone else support that idea so I will leave the setting in.

Here is a new patch.

Anybody’s picture

Thank you AdamPS, the patch / interdiff looks good. Thank you for pointing out that missing piece.
I'm not sure if true should be the default for css_inliner. Perhaps we should discuss the default state? What do the others think what a user expects?

Technically RTBC +1

AdamPS’s picture

Perhaps we should discuss the default state?

  • Default = true is good for the majority of cases because inline CSS just works better.
  • Default = false is better for back-compatibility.
  • But personally I still prefer to remove the config setting altogether. Then it is back-compatible (nothing happens until you put CSS in the library), but also works better (if you put CSS in the library then it is automatically inline rather than needing to turn that on separately).
AdamPS’s picture

Here is an alternative, simpler patch that removes the config setting. Hopefully this way should just "do the right thing" for everyone

  • For existing sites that are working fine, this patch is safe. It's back-compatible as nothing happens until you put CSS in the library.
  • For sites that need to add extra CSS via the library, this patch is simple: just alter the library, no extra config setting to enable.
  • For developers needing to debug, I have raised a new issue #3015949: Allow debugging of CSS.

The problem with the setting was that although the description was

Convert CSS to inline styles to assure solid rendering in all email clients.

in the previous patch there was a confusing hidden meaning: if you turn this setting off, CSS from the library is silently discarded.

Anybody’s picture

RTBC +1 for #61 generally. It works perfectly and I think it's good.

BUT (sorry) the change to the README.txt doesn't seem to be correct:

+Add custom css to the template with the preprocess hook and convert them to
+inline-styles with the corresponding option under the message settings.

So I'd suggest two last changes:

  • Add an information to the README.txt how to add inline stylesheets, perhaps with a programmatic example (template/swiftmailer library)
  • Perhaps even add a short information about inline styles to the admin config page below the HTML format?

That's it!

AdamPS’s picture

@Anybody Your suggestions for the README.txt makes sense. I don't have time for another patch right now, but feel free to go ahead or maybe someone else can.

AdamPS’s picture

New patch with updated README. I'll be away from Drupal.org for a while now so if any more work is needed hopefully other people can join in.

dimr’s picture

Status: Needs review » Reviewed & tested by the community

I am using the patch #64 with the dev version of the module and everything is working fine. I have inline images and inline CSS automatically done by the theme.
I have read and followed the instructions described in the README and everything was clear to me.
Thanks a good work!

Stefdewa’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
10.15 KB

Rerolled patch against latest dev.

zengenuity’s picture

Status: Needs review » Reviewed & tested by the community

Re-rolled patch works for me on latest dev.

Anybody’s picture

Confirming RTBC!
Great, can we have a commit and release for this? Any active maintainer?

Anybody’s picture

Can we have a commit and new release please? This is really important for many users of the module...

ndf’s picture

+1 RTBC

With this patch it is super-easy to add custom inline css to an outgoing e-mail.

I followed the updated README to add a specific my_theme.mail.css to my theme in my_theme.libraries.yml:

swiftmailer:
  css:
    theme:
      css/my_theme.mail.css: {}

Note: in my opinion mail.css is a better name then my_theme.mail.css

In my_theme.mail.css I added some basic css.

In admin/config/system/mailsystem I set the custom theme as default as "Theme to render the emails".
Then I sended a test-mail on admin/config/swiftmailer/test

The received mail did contain the new css.

Diving into the received mail source, I saw that this css was indeed inlined.
my_theme.mail.css contained div { background-color: #dd6b63; }
In gmail this became: <div style="background-color:#dd6b63">. Exactly as expected.

Note that because of https://github.com/cweagans/composer-patches/issues/233 I had to manually add the new requirement tijsverkoyen/css-to-inline-styles:^2.2
Another note this patch now adds drupal/mailsystem": "^4" to composer.json. This seems to be fine because mailsystem is a requirement.

bohart’s picture

+1 to RTBC,

added patch to the current project. It works like a charm.

In the meantime, patched README file described how to add CSS file by theme.
I would like to suggest to add how to apply styles by the module.
Cuz it's super easy with the current patch:

/**
 * Implements hook_preprocess_HOOK().
 */
function MYMODULE_preprocess_swiftmailer(&$variables) {
  // Process for emails by this module.
  if ($variables['message']['module'] !== 'MYMODULE') {
    return;
  }

  // Add own styles with a library.
  $variables['#attached']['library'][] = 'MYMODULE/email_styles';
}

I have updated the appropriate README section,
the updated patch added.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 71: swiftmailer-css_inliner-2843327-71-D8.patch, failed testing. View results

andrewbelcher’s picture

Not sure what has changed that results in the last patch failing, but I have tried the patch from #66 and it is working great for me! Really useful feature for swiftmailer!! So +1 RTBC as far as functionality goes!

ndf’s picture

Status: Needs work » Needs review

Trigger testbot

AdamPS’s picture

Status: Needs review » Reviewed & tested by the community

Test fail in #72 was just a glitch. Latest patch is only different in README file. It looks good to me.

Anybody’s picture

Thank you very much. Could someone please contact an active maintainer?

andyg5000’s picture

Hopefully @webflow will see this and commit it with the next release. Clearly a lot of people need this. Thanks for all the effort on this!

Anybody’s picture

Triggered retest. I guess the patch needs a reroll after @webflos latest dev release two days ago. Hopefully this will then be part of the next dev release :)

@bohart would you be so kind to do the reroll?

Anybody’s picture

Status: Reviewed & tested by the community » Needs work
pjbaert’s picture

>@bohart would you be so kind to do the reroll?

Sorry to hijack this issue, but since I needed to patch this for a project, I did the reroll myself.
Fire up the test bots!

Anybody’s picture

Status: Needs review » Reviewed & tested by the community

Great, thank you pjbaert! :)

Confirming RTBC again for #80. Could someone perhaps contact a maintainer to get this into the next dev release? It's time I think... ;)

webflo’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, it took to log to review this patch. But i have to set it back to needs work.

+++ b/src/Plugin/Mail/SwiftMailer.php
@@ -85,13 +119,25 @@ class SwiftMailer implements MailInterface, ContainerFactoryPluginInterface {
+    $this->libraryDiscoverParser = $library_discover_parser;

$this->libraryDiscoverParser is unused.

libraryDiscoverParser is unused.

+++ b/src/Plugin/Mail/SwiftMailer.php
@@ -126,9 +174,20 @@ class SwiftMailer implements MailInterface, ContainerFactoryPluginInterface {
+          'library' => ["$mail_theme/swiftmailer"],

What happens if the theme does not define the swiftmailer lib? Does Drupal fail silent? I think we should catch that case. Hence the libraryDiscoverParser?

+++ b/src/Plugin/Mail/SwiftMailer.php
@@ -126,9 +174,20 @@ class SwiftMailer implements MailInterface, ContainerFactoryPluginInterface {
       $render = [

themeManager is not defined in this class. Please add it to constructor

AdamPS’s picture

@webflo Thanks for the review.

1) I think libraryDiscoverParser is left over from some earlier attempts at the patch and I removed it.

2) It is no problem if the theme doesn't define the swiftmailer lib - getCssAssets() will return an empty array. So we already catch this case with if ($css). I don't see it as a "fail" or "error" case as the swiftmailer lib is optional, so I don't think it's correct to log/warn.

3) Well spotted I added themeManager.

Anybody’s picture

#83 works perfect. RTBC+1

geek-merlin’s picture

Status: Needs review » Reviewed & tested by the community

The interdiff shows that changes are trivial and address #82. Setting RTBC as of #84.

borisson_’s picture

Yes, the latest patch does look very good. All remarks have been processed.

I agree with #83.2.

2) It is no problem if the theme doesn't define the swiftmailer lib - getCssAssets() will return an empty array. So we already catch this case with if ($css). I don't see it as a "fail" or "error" case as the swiftmailer lib is optional, so I don't think it's correct to log/warn.

We are using #80 on almost all of our projects, often as the only patch and have had it working perfectly, so +1 from me.

TommyChris’s picture

I adopted the patch from #83 for branch 2.x.

FMB’s picture

Could this patch be committed now? Thanks!

Anybody’s picture

Any active module maintainer and plans for a commit and new release? As you can see from this the many comments, this is quite important.

Thank you!

HabibMosavi’s picture

Hi there!
Just used the library today, works great but faced a small issue:
It doesn't embed the CSS position property, EX: position: absolute;

Thanks.

geek-merlin’s picture

> Just used the library today, works great but faced a small issue: It doesn't embed the CSS position property, EX: position: absolute;

Thanks for the information. If i get it right, this should go upstream to the library.

AdamPS’s picture

Yes I agree - the specific detail of CSS inlining is handled by the external library code. There is no way to fix the problem you describe by changing the patch. If you would like the problem fixed please raise a bug with the external library.

Note that Openbuildings\Swiftmailer\CssInlinerPlugin is just a simple wrapper for TijsVerkoyen\CssToInlineStyles\CssToInlineStyles so I think you need to raise an issue here.

AdamPS’s picture

Status: Reviewed & tested by the community » Needs work

When checking the code as to write my previous comment I just spotted that there seems to be a duplicated require of drupal/mailsystem - I'm away from my desk so if anyone can fix that it would be great.

--- a/composer.json
+++ b/composer.json
@@ -4,9 +4,11 @@
     "type": "drupal-module",
     "license": "GPL-2.0+",
     "require": {
+      "drupal/mailsystem": "^4",
       "egulias/email-validator": "~2.0",
       "drupal/mailsystem": "^4.1.0",
       "html2text/html2text": "~4.0.1",
+      "tijsverkoyen/css-to-inline-styles": "^2.2",
       "php": ">=7.0.0",
       "swiftmailer/swiftmailer": "~6.1.3"
mpaler’s picture

@ndf or anybody else...

How did you "manually add the new requirement tijsverkoyen/css-to-inline-styles:^2.2"?

I can patch the module, however, i can't get the new library to install... What's the solution here?

Solution found here: https://github.com/cweagans/composer-patches#patches-containing-modifica...

AdamPS’s picture

Status: Needs work » Reviewed & tested by the community

Sorry my mistake. The patch in #83 is fine.

The comment in #93 is valid regarding the 2.x patch in #87. I admit I'm not sure what the situation is with the 2.x version of this module - whether it is ready for use yet or just experimental. If the maintainers prefer a patch for 2.x then backport then please let us know.

PhilippVerpoort’s picture

Patch in #87 applied without problems and seems to be working as expected.

Please commit to branch and release soon. Many thanks to everybody for their hard work.

ultimike’s picture

Patch #87 looks good to me - please commit :)

-mike

KarenS’s picture

I ran into this very handy issue and have a couple comments. The suggested methods for adding css to the regular theme with "magic" names is not really consistent with Drupal's regular theme system. Instead I would recommend creating a new theme that is a sub theme of your regular theme. Add email css overrides and the SwiftMailer template to that new theme. There are lots of changes you will probably have to make to get email working right on various clients, so you'll probably need the flexibility of a new theme anyway. You can do other things there, like adding simpler node templates, creating simple page header, etc. So I would get rid of that part of the patch and change the instructions slightly to tell users to create an email theme and set that as the theme the mail system should use.

I ran into the same problem where the new external library won't work if it's added in a patch, so you need to add it manually to test it.

Still poking around, but this work looks really useful!

KarenS’s picture

Ugh, I see the problem. Swiftmailer ignores the theme you set in Mailsystem, so no point creating one :(

KarenS’s picture

FWIW, you can do this without a patch using hooks:

<?php

use Drupal\Core\Asset\AttachedAssets;
use TijsVerkoyen\CssToInlineStyles\CssToInlineStyles;

/**
 * Implements hook_swiftmailer_alter().
 */
function message_integration_swiftmailer_alter(Swift_Mailer &$swiftMailer, Swift_Message &$swiftMessage, $message) {

  // Mail and Swiftmail ignore libraries when rendering messages. So we identify
  // some  css libraries and attach them here.
  $render = [
    '#attached' => [
      'library' => [
        'core/normalize',
        'bartik/global-styling',
      ],
    ],
  ];
  $assets = AttachedAssets::createFromRenderArray($render);

  // For email we don't want linked css files in the HEAD of the page, we
  // want css to be inlined into the body. So we construct a single string of
  // css, then use CssToInlineStyles() to render that css inline in the text.
  $assetResolver = \Drupal::service('asset.resolver');
  $fileSystem = \Drupal::service('file_system');

  $css = '';
  foreach ($assetResolver->getCssAssets($assets, FALSE) as $css_asset) {
    $css .= file_get_contents($fileSystem->realpath($css_asset['data']));
  }

  $text = $swiftMessage->getBody();
  if ($css) {
    $text = (new CssToInlineStyles())->convert($text, $css);
    $swiftMessage->setBody($text);
  }
}
GlarDup’s picture

I tried to apply the patch but none of them work. (composer 'could not apply patch' error). (D8.8.1) I have no idea why.

FMB’s picture

tried to apply the patch but none of them work. (composer 'could not apply patch' error). (D8.8.1) I have no idea why.

I guess the patch needs a re-roll.

We should also decide if we adopt it as it was submitted, or if we need to take KarenS' remarks (which I'm not sure I understood^^) into account.

AdamPS’s picture

I tried to apply the patch but none of them work. (composer 'could not apply patch' error). (D8.8.1) I have no idea why.

The patch still works fine for me on D8.8.2. As far as I can see there have been no commits to swiftmailer for many months so a reroll should not be needed.

We should also decide if we adopt it as it was submitted, or if we need to take KarenS' remarks (which I'm not sure I understood^^) into account.

As far as I can see #100 is a workaround - it explains how to write custom code that does the same as this patch. Once this patch has been committed then there would be no need for the workaround.

FMB’s picture

As far as I can see #100 is a workaround - it explains how to write custom code that does the same as this patch. Once this patch has been committed then there would be no need for the workaround.

Thanks for the explanation.

The patch still works fine for me on D8.8.2. As far as I can see there have been no commits to swiftmailer for many months so a reroll should not be needed.

GlarDup, you could try running Composer in verbose mode, adding options such as -v, -vv or -vvv. Otherwise, you can try to apply the patch with Git or with the "patch" command.

GlarDup’s picture

Issue summary: View changes

This is what I get with patch #87. Should I have applied other patches before?

https://www.drupal.org/files/issues/2019-05-25/swiftmailer.css_inliner.2... (inline css)
patch '-p1' --no-backup-if-mismatch -d 'web/modules/contrib/swiftmailer' < '/tmp/5e3af4cd44899.patch'
patching file README.txt

patching file composer.json

Hunk #1 FAILED at 4.

1 out of 1 hunk FAILED -- saving rejects to file composer.json.rej

patching file src/Plugin/Mail/SwiftMailer.php

Hunk #1 FAILED at 5.

Hunk #2 succeeded at 30 with fuzz 2 (offset 5 lines).
Hunk #3 succeeded at 80 with fuzz 2 (offset 7 lines).

Hunk #4 FAILED at 114.

Hunk #5 succeeded at 117 (offset -15 lines).
Hunk #6 succeeded at 142 (offset -15 lines).
Hunk #7 succeeded at 164 with fuzz 1 (offset -15 lines).

Hunk #8 succeeded at 237 (offset 10 lines).

Hunk #9 FAILED at 350.

Hunk #10 succeeded at 615 (offset 111 lines).
Hunk #11 succeeded at 655 (offset 111 lines).
Hunk #12 succeeded at 696 (offset 111 lines).

3 out of 12 hunks FAILED -- saving rejects to file src/Plugin/Mail/SwiftMailer.php.rej

patch '-p0' --no-backup-if-mismatch -d 'web/modules/contrib/swiftmailer' < '/tmp/5e3af4cd44899.patch'
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/README.txt b/README.txt
|index 35b10f8..793b33b 100644
|--- a/README.txt
|+++ b/README.txt
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.

2 out of 2 hunks ignored

can't find file to patch at input line 74
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/composer.json b/composer.json
|index 78fe6fa..7f1c1d0 100644
|--- a/composer.json
|+++ b/composer.json
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.

1 out of 1 hunk ignored

can't find file to patch at input line 90
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/src/Plugin/Mail/SwiftMailer.php b/src/Plugin/Mail/SwiftMailer.php
|index 528e957..37fd36a 100644
|--- a/src/Plugin/Mail/SwiftMailer.php
|+++ b/src/Plugin/Mail/SwiftMailer.php
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.

12 out of 12 hunks ignored

patch '-p2' --no-backup-if-mismatch -d 'web/modules/contrib/swiftmailer' < '/tmp/5e3af4cd44899.patch'
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/README.txt b/README.txt
|index 35b10f8..793b33b 100644
|--- a/README.txt
|+++ b/README.txt
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.

2 out of 2 hunks ignored
can't find file to patch at input line 74
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/composer.json b/composer.json
|index 78fe6fa..7f1c1d0 100644
|--- a/composer.json
|+++ b/composer.json
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.

1 out of 1 hunk ignored

can't find file to patch at input line 90
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/src/Plugin/Mail/SwiftMailer.php b/src/Plugin/Mail/SwiftMailer.php
|index 528e957..37fd36a 100644
|--- a/src/Plugin/Mail/SwiftMailer.php
|+++ b/src/Plugin/Mail/SwiftMailer.php
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.

12 out of 12 hunks ignored

patch '-p4' --no-backup-if-mismatch -d 'web/modules/contrib/swiftmailer' < '/tmp/5e3af4cd44899.patch'
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/README.txt b/README.txt
|index 35b10f8..793b33b 100644
|--- a/README.txt
|+++ b/README.txt
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.

2 out of 2 hunks ignored
can't find file to patch at input line 74
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/composer.json b/composer.json
|index 78fe6fa..7f1c1d0 100644
|--- a/composer.json
|+++ b/composer.json
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.

1 out of 1 hunk ignored

can't find file to patch at input line 90
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/src/Plugin/Mail/SwiftMailer.php b/src/Plugin/Mail/SwiftMailer.php
|index 528e957..37fd36a 100644
|--- a/src/Plugin/Mail/SwiftMailer.php
|+++ b/src/Plugin/Mail/SwiftMailer.php
--------------------------
File to patch:
Skip this patch? [y]
Skipping patch.

12 out of 12 hunks ignored

geek-merlin’s picture

Status: Reviewed & tested by the community » Needs work

So it looks like #87 does not apply anymore and needs a reroll. If that reroll does not add significant changes (so better add an interdiff) IMHO it may go RTBC directly.

FMB’s picture

Status: Needs work » Reviewed & tested by the community

So it looks like #87 does not apply anymore and needs a reroll.

I'm not so sure. I can apply it on 8.x-1.x. Besides, according to AdamPS, nothing has changed recently. Since the whole patch is rejected on GlarDup's installation, I believe something else is happening. Are you using the right version for this module? Are you in the right directory when you're trying to apply the patch?

geek-merlin’s picture

I just made some issue gardener's work and this is how i understood #106. So let's see what the Bot says, triggered it.

geek-merlin’s picture

Status: Reviewed & tested by the community » Needs work

That was quick:
https://www.drupal.org/pift-ci-job/1567297
Patch Failed to Apply

KarenS’s picture

A couple comments:

- My workaround above has the added benefit of allowing you to include core libraries, like core/normalize. The patch does not. You can have only one library and if you want stuff from core you'll have to copy all of it into your library.
- It looks like there is an assumption that you will have a library called swiftmailer. I think it will fail if you do not, so it's not even optional.

I would get rid of the hard-coded assumption that there is only one library, and that it must be called 'swiftmailer'. You could create a config entity to allow the user to list the libraries they want to use so core libraries can be used.

GlarDup’s picture

@KarenS
Agreed.
Unfortunately I couldn't get the workaround to work. The hook is never called.

What I did
- composer required the TijsVerkoyen library
- added the hook to my theme file
-> email works fine, but theme is not inserted (nothing at all, not as css nor inline)
- replaced the core and bartik libraries by my own (very simple) email library in the hook's render array.
-> no change
- cleared the cache a few times extra, just to be sure + checked naming
-> nope
- put $message="hook"; in the hook as a lazy way of testing whether it actually ever gets fired
-> nothing. No log message or error. Email is sent unaltered.

Things I double checked
- Swiftmailer does the formatting and sending
- Swiftmailer twig is used
- The other hooks in my theme work just fine
- The name of the hook is mytheme_swiftmailer_alter

Did I forget something?

AdamPS’s picture

@KarenS It's true we could enhance the patch to allow configurable libraries and allow multiple libraries. Personally I don't have those needs so I won't be the one to do it. Equally we could commit the patch as it, and add that flexibility later if no one else volunteers to write the code now.

My feeling is that libraries are a developer concept and not necessarily appropriate to configure in the admin UI. Perhaps a better approach would be to have an alter hook? The default would be ['swiftmailer'].

FMB’s picture

I agree with AdamPS. This issue has now been open for more than three years. Since we focused quite early on Css Inliner, it would make sense to commit this patch and open another issue to allow configurable libraries if desired.

Personally all I need is inline styles when asked by my clients (who don't understand why this is not proposed by default). I'm not sure I would find necessary to use another library.

KarenS’s picture

If the hook never gets called it is probably not named correctly. In my example I call it 'message_integration_swiftmailer_alter' because the module I use it in is named 'message_integration'.

iainH’s picture

I agree with #113 and #114
The essential inline styles feature is the immediate requirement. Would be great if that were committed soon.

geek-merlin’s picture

Status: Needs work » Reviewed & tested by the community

OK i have to correct my earlier comments.

I could manually verify

* #83 applies on 1.x
* #87 applies on 2.x

So according to #95 this would be RTBC.

In #110 though the problem was mentioned that the $mailtheme/seiftmailer library is attached whether it is defined or not. I do not know if core coughs when an undefined library is arrached, but it might do anytime in the future, so we must not do that.

So the patch IMHO needs to be changed so the library is attached ONLY when it is defined.

AdamPS’s picture

Thanks @geek-merlin

In #110 though the problem was mentioned that the $mailtheme/swiftmailer library is attached whether it is defined or not. I do not know if core coughs when an undefined library is attached,

I believe I tested this case and it is fine.

, but it might do anytime in the future, so we must not do that.

My thought is why would someone remove some code that safeguards against a particular condition? That would be a non-back-compatible change. The Drupal Backward compatibility (BC) policy says:

We will take reasonable efforts to not break code that developers may be relying on.

If you are really enthusiastic to write this extra code then I wouldn't object. However to me it seems to be making the code more complex without providing much benefit.

geek-merlin’s picture

> I believe I tested this case and it is fine.
> [Having that changed] would be a non-back-compatible change.

OK i agree, no objections to RTBC.

Anybody’s picture

Agree with AdamPS & geek-merlin! RTBC++ ;)

geek-merlin’s picture

Just crosschecked in the source: Non-defined libraries are silently ignored. LibraryDiscovery::getLibraryByName returns FALSE, and AssetResolver::getFooAssets does not cough on this. If someone(tm) likes, we can add a comment about that.

geek-merlin’s picture

Let's see if this applies on 2.x.

geek-merlin’s picture

Version: 8.x-1.x-dev » 8.x-2.x-dev
Status: Reviewed & tested by the community » Needs work

This needs re-roll for 2.x.

geek-merlin’s picture

I found something more:

+++ b/src/Plugin/Mail/SwiftMailer.php
@@ -137,6 +196,17 @@ class SwiftMailer implements MailInterface, ContainerFactoryPluginInterface {
+        $css .= file_get_contents($this->fileSystem->realpath($css_asset['data']));

'realpath' will break for non-local stream wrappers. Please remove.

geek-merlin’s picture

AdamPS’s picture

Thanks. Let's wait for commit of #2812941: Make URLs in links and images absolute which I just re-rolled otherwise this will need re-roll again.

AdamPS’s picture

'realpath' will break for non-local stream wrappers. Please remove.

Please can you explain what to do instead?

geek-merlin’s picture

+        $css .= file_get_contents($css_asset['data']);

I'm working on similar things in a core issue so this hit my eyes.

AdamPS’s picture

Title: Support CSS inlining » Load CSS from a theme with CSS inlining

Good news here is that @geek-merlin and I are now maintainers so this will get in and it won't be a long wait.

It means that I need to look at the patch with fresh eyes.

  • Probably this doesn't load CSS files correctly. Look at all the stuff done by CssOptimizer: @import, relative path, path prefixing, BOM. I think we can pull that in by enabling the $optimize parameter of getCssAssets().
  • We should add a test.

  • AdamPS committed d5ce24e on 8.x-2.x
    Issue #2843327 by AdamPS: Coding standards fixes
    
AdamPS’s picture

Status: Needs work » Needs review
FileSize
7.89 KB

OK I committed the coding standards parts which should make it clearer what the actual fix is.

I re-rolled the patch for 2.x and applied the comment #123 and first one from #128. We can't add tests until after #3124110: Fix/improve the tests for SwiftMailer::format.

Sorry no interdiff because of the change from 1.x to 2.x.

AdamPS’s picture

geek-merlin’s picture

#128:
> Probably this doesn't load CSS files correctly. Look at all the stuff done by CssOptimizer: @import, relative path, path prefixing, BOM. I think we can pull that in by enabling the $optimize parameter of getCssAssets().

Good catch! It will work for the simplest self-contained cases, but any @import will break due to an unknown relative path when the css is used outside its web path. And this resolving of relative links is what CssOptimizer emulates.

Sooner or later someone will request to remove the optimization so the css can be debugged better. So we should add a comment like so (to that source line plus API docs...

CSS may contain references to other files (css, images, ...). We leverage the core CSS optimizer to resolve any relative file references, which we have to as we pull that CSS out of its HTTP request context.
If you want to suppress optimizing (e.g. for debugging purposes), you can set 'preprocess: false' for some CSS assets. But be warned that any relative file references will break then.
AdamPS’s picture

Thank for the review. I agree with your idea and I already added a similar comment

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

It has the advantage of being simple and general - it hints at the action of CssOptimizer without trying to precisely list is allowing that it might change in later releases. It gives the key message: we are optimising for a reason and if you turn it off then you can expect problems.

You alternative seems more precise and detailed, but the details are tricky and maybe not really needed?

It's not just @include there is other processing in there too including BOM/character set conversion. I can understand your idea when you say the CSS is "pulled out of its HTTP request content" and "relative references will break" - it seems you are thinking the same as why we convert relative links to absolute. However it's more that that - the CSS about to be inlined and CssToInlineStyles doesn't process @include as far as I can see.

It's a fair point to help people for debugging, but maybe there's a better place to explain it than comments in the code. I raised #3015949: Allow debugging of CSS and you persuaded me to close it again:-).

Would be great to get a commit here. Or if you are still not happy with my comment then maybe you can suggest another variation that includes the best of both points of view??

geek-merlin’s picture

Status: Needs review » Reviewed & tested by the community

This was just a quick idea and suggestion for the docs. Patch is good enough to go a i am concerned, and we can improve docs anytime later.

  • AdamPS committed e6b2a3f on 8.x-2.x
    Issue #2843327 by AdamPS, heddn, andyg5000, FMB, pjbaert, bohart, dimr,...
AdamPS’s picture

Status: Reviewed & tested by the community » Fixed

Great here is the commit - finally! Thanks to everyone who helped.

agoradesign’s picture

wow! that's great :))) many thanks to both of you for your great work on this module!

Anybody’s picture

Yes, thank you all very very much.... one of the projects / issues patched since years =D I'll not miss it ...

Status: Fixed » Closed (fixed)

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

mrdrill’s picture

I've been using patch swiftmailer.css_inliner.2843327-131.patch and it's been great until I updated to Drupal core 8.9.16 and it stopped working and my css is no longer inline. I tried reapplying the patch but that does not seem to solve the problem.

FMB’s picture

@mrdill Maybe the explanations given in the README file in section 2.1 can help you.

mrdrill’s picture

@FMB thanks for the suggestion I tried a few ways in my_theme.libraries.yml

swiftmailer:
css:
theme:
css/mail.css: {}

swiftmailer:
css:
theme:
css/my_theme.mail.css: {} replacing my_theme with my actual theme name

I also tried #100 as a workaround

but nothing seems to take i've verified I have all the right libraries still installed, I'm not seeing how the update to 8.9.16 broke this