Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#131 | swiftmailer.css_inliner.2843327-131.patch | 7.89 KB | AdamPS |
| |||
#80 | swiftmailer-css_inliner-2843327-80-D8.patch | 10.7 KB | pjbaert |
| |||
#83 | swiftmailer.css_inliner.2843327-83.patch | 10.67 KB | AdamPS |
Comments
Comment #2
Lukas von BlarerYes, this would be a very useful feature.
Comment #3
FMB CreditAttribution: FMB commentedAs 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:
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.
Comment #4
Lukas von BlarerGreat, 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.
Comment #5
christian-heiko CreditAttribution: christian-heiko at Liip commentedthis is a patch which gives the option to activate tijsverkoyen/css-to-inline-styles within the format of swiftmailer
Comment #6
Lukas von Blarer@christian-heiko cool, I am going to test this soon on my current project.
Comment #7
FMB CreditAttribution: FMB commentedUpdated 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.
Comment #8
dimr CreditAttribution: dimr commentedI 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.
Comment #9
dimr CreditAttribution: dimr commentedComment #10
andyg5000Comment #11
andyg5000@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:
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.
Comment #13
andyg5000Comment #14
zengenuity CreditAttribution: zengenuity at DrupalTutor / Zengenuity commentedThis patch works for me, but it needs a re-roll to apply cleanly to dev.
Comment #15
PhilippVerpoortI can confirm that adding the library as explained in #11 and applying the patch in #14 has worked for me.
Comment #16
osopolarThe 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.
Comment #17
agoradesign CreditAttribution: agoradesign commented@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 :)
Comment #18
Lukas von BlarerIf i remember correctly, running another composer update after applying the patch, fixes the issue.
Comment #19
yareckon CreditAttribution: yareckon commentedCouldn'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
Comment #20
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedNice patch. Comments:
#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.
Comment #21
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedPatch that fixes my own comments, plus I spotted that the form #description needed updating
Comment #22
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedDo we actually need the form setting css_inliner?
Simpler patch that remove the form setting.
Comment #23
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedPS Does anyone know why the tests are failing? Could it be because of the extra composer dependency and the same problem as #16?
Comment #26
FMB CreditAttribution: FMB commentedFor debugging purposes, for instance. Mimemail has a CSS compressor module you can choose to enable or not.
Comment #27
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@FMB Please can you clarify what you would prefer?
Comment #28
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedI should explain more clearly.
The patch in #14 has a setting 'css_inliner' with description:
As far as I can see, it behaves like this:
I think what you are expecting, and what the description suggests for is more like this:
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??
Comment #29
FMB CreditAttribution: FMB commentedAdamPS: 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.
Comment #30
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@FMB OK, thanks for the clarification.
The complication is that the patch in this issue (#14 onwards) actually does two things:
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.
Comment #31
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedOK, 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
Comment #33
heddnI'm hoping this will fix the failing tests.
Comment #34
heddnIt 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.
Comment #35
heddnNo more CS failures. All tests are passing. Do we want/need tests about inlining the CSS?
Comment #36
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@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.
Comment #37
pmoncel CreditAttribution: pmoncel commentedPatch is OK for me : css inlines are well pushed in the html code and remain in the head of the message.
Comment #38
heddnSo, 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.
Comment #39
AnybodyGreat! When will we see a new dev release containing this? Confirming RTBC :)
Comment #40
AnybodyI 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.
Comment #41
agoradesign CreditAttribution: agoradesign commentedStrange, because the project is definitely registered on packagist.org: https://packagist.org/packages/tijsverkoyen/css-to-inline-styles
Comment #42
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@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.
Comment #43
AnybodySorry @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!
Comment #44
agoradesign CreditAttribution: agoradesign commentedno 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
Comment #45
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedI propose this as a key issue for the next release as newsletters don't really work without it.
Comment #46
AnybodyYes that's perfect. At least for stone-age mail programs like outlook ;)
Comment #47
AnybodyConfirming RTBC after using the patch for weeks on several sites now. Can we get this into the dev next release / stable?
Comment #48
AnybodyAny 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.
Comment #49
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedMinor fix to avoid inserting a zero-length style header.
Comment #50
FMB CreditAttribution: FMB commentedAdamPS: could you provide an interdiff? Makes reviews easier, thanks.
Comment #51
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedUnfortunately 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.
Comment #52
FMB CreditAttribution: FMB commentedTested last patch on a clean Drupal 8 install, looks good to me, thanks.
Comment #53
AnybodyRetesting #49 against Core 8.7.x and 8.6.x. Is there an active maintainer for this project who can commit this afterwards?
Comment #54
heddnLooks like tests are failing?
Comment #55
Anybody@heddn, please test #49 manually. To me it looks like failing tests are unrelated.
I guess the testbot can't handle the dependency:
We're using the patch since month in our composer.json patches (automatically) without any problems.
Comment #56
FMB CreditAttribution: FMB commentedThere seem to be some minor coding standards issues, though.
Comment #57
FMB CreditAttribution: FMB commentedReally minor indeed! I hope Anybody's explanations suit you ^^
Comment #58
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedI 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
We need to fix that with an update hook.
2)
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.
Comment #59
AnybodyThank 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
Comment #60
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedComment #61
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedHere is an alternative, simpler patch that removes the config setting. Hopefully this way should just "do the right thing" for everyone
The problem with the setting was that although the description was
in the previous patch there was a confusing hidden meaning: if you turn this setting off, CSS from the library is silently discarded.
Comment #62
AnybodyRTBC +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:
So I'd suggest two last changes:
That's it!
Comment #63
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@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.
Comment #64
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedNew 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.
Comment #65
dimr CreditAttribution: dimr at FIZ Karlsruhe commentedI 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!
Comment #66
Stefdewa CreditAttribution: Stefdewa at Calibrate commentedRerolled patch against latest dev.
Comment #67
zengenuity CreditAttribution: zengenuity at DrupalTutor / Zengenuity commentedRe-rolled patch works for me on latest dev.
Comment #68
AnybodyConfirming RTBC!
Great, can we have a commit and release for this? Any active maintainer?
Comment #69
AnybodyCan we have a commit and new release please? This is really important for many users of the module...
Comment #70
ndf CreditAttribution: ndf at Dx Experts commented+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:
Note: in my opinion
mail.css
is a better name thenmy_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.Comment #71
bohart+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:
I have updated the appropriate README section,
the updated patch added.
Comment #73
andrewbelcher CreditAttribution: andrewbelcher at FreelyGive commentedNot 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!
Comment #74
ndf CreditAttribution: ndf at Dx Experts commentedTrigger testbot
Comment #75
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedTest fail in #72 was just a glitch. Latest patch is only different in README file. It looks good to me.
Comment #76
AnybodyThank you very much. Could someone please contact an active maintainer?
Comment #77
andyg5000Hopefully @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!
Comment #78
AnybodyTriggered 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?
Comment #79
AnybodyComment #80
pjbaert>@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!
Comment #81
AnybodyGreat, 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... ;)
Comment #82
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedSorry, it took to log to review this patch. But i have to set it back to needs work.
$this->libraryDiscoverParser
is unused.libraryDiscoverParser is unused.
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?
themeManager is not defined in this class. Please add it to constructor
Comment #83
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@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 withif ($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.
Comment #84
Anybody#83 works perfect. RTBC+1
Comment #85
geek-merlinThe interdiff shows that changes are trivial and address #82. Setting RTBC as of #84.
Comment #86
borisson_Yes, the latest patch does look very good. All remarks have been processed.
I agree with #83.2.
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.
Comment #87
TommyChrisI adopted the patch from #83 for branch 2.x.
Comment #88
FMB CreditAttribution: FMB commentedCould this patch be committed now? Thanks!
Comment #89
AnybodyAny 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!
Comment #90
HabibMosavi CreditAttribution: HabibMosavi commentedHi 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.
Comment #91
geek-merlin> 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.
Comment #92
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedYes 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 forTijsVerkoyen\CssToInlineStyles\CssToInlineStyles
so I think you need to raise an issue here.Comment #93
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedWhen 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.
Comment #94
mpaler CreditAttribution: mpaler commented@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...
Comment #95
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedSorry 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.
Comment #96
PhilippVerpoortPatch 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.
Comment #97
ultimikePatch #87 looks good to me - please commit :)
-mike
Comment #98
KarenS CreditAttribution: KarenS at Lullabot commentedI 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!
Comment #99
KarenS CreditAttribution: KarenS at Lullabot commentedUgh, I see the problem. Swiftmailer ignores the theme you set in Mailsystem, so no point creating one :(
Comment #100
KarenS CreditAttribution: KarenS at Lullabot commentedFWIW, you can do this without a patch using hooks:
Comment #101
GlarDupI tried to apply the patch but none of them work. (composer 'could not apply patch' error). (D8.8.1) I have no idea why.
Comment #102
FMB CreditAttribution: FMB commentedI 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.
Comment #103
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThe 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.
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.
Comment #104
FMB CreditAttribution: FMB commentedThanks for the explanation.
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.
Comment #105
GlarDupThis 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
Comment #106
geek-merlinSo 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.
Comment #107
FMB CreditAttribution: FMB commentedI'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?
Comment #108
geek-merlinI just made some issue gardener's work and this is how i understood #106. So let's see what the Bot says, triggered it.
Comment #109
geek-merlinThat was quick:
https://www.drupal.org/pift-ci-job/1567297
Patch Failed to Apply
Comment #110
KarenS CreditAttribution: KarenS at Lullabot commentedA 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.
Comment #111
GlarDup@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?
Comment #112
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@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'].
Comment #113
FMB CreditAttribution: FMB commentedI 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.
Comment #114
KarenS CreditAttribution: KarenS at Lullabot commentedIf 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'.
Comment #115
iainH CreditAttribution: iainH commentedI agree with #113 and #114
The essential inline styles feature is the immediate requirement. Would be great if that were committed soon.
Comment #116
geek-merlinOK 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.
Comment #117
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThanks @geek-merlin
I believe I tested this case and it is fine.
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:
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.
Comment #118
geek-merlin> 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.
Comment #119
AnybodyAgree with AdamPS & geek-merlin! RTBC++ ;)
Comment #120
geek-merlinJust 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.
Comment #121
geek-merlinLet's see if this applies on 2.x.
Comment #122
geek-merlinThis needs re-roll for 2.x.
Comment #123
geek-merlinI found something more:
'realpath' will break for non-local stream wrappers. Please remove.
Comment #124
geek-merlinFYI
Comment #125
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThanks. 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.
Comment #126
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedPlease can you explain what to do instead?
Comment #127
geek-merlinI'm working on similar things in a core issue so this hit my eyes.
Comment #128
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedGood 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.
getCssAssets()
.Comment #130
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedOK 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.
Comment #131
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedComment #132
geek-merlin#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...
Comment #133
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThank for the review. I agree with your idea and I already added a similar comment
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??
Comment #134
geek-merlinThis 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.
Comment #136
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedGreat here is the commit - finally! Thanks to everyone who helped.
Comment #137
agoradesign CreditAttribution: agoradesign commentedwow! that's great :))) many thanks to both of you for your great work on this module!
Comment #138
AnybodyYes, thank you all very very much.... one of the projects / issues patched since years =D I'll not miss it ...
Comment #140
mrdrill CreditAttribution: mrdrill commentedI'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.
Comment #141
FMB CreditAttribution: FMB commented@mrdill Maybe the explanations given in the README file in section 2.1 can help you.
Comment #142
mrdrill CreditAttribution: mrdrill commented@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