Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

addExcludedSelector does not exclude everything that is in selector #347

Closed
Voyteck opened this issue Sep 19, 2016 · 15 comments · Fixed by #768
Closed

addExcludedSelector does not exclude everything that is in selector #347

Voyteck opened this issue Sep 19, 2016 · 15 comments · Fixed by #768

Comments

@Voyteck
Copy link

Voyteck commented Sep 19, 2016

I use Emgrifier to create confirmation mail for email messages sent to user. Therefore I have an email that contains a section, in which contents of another email (email preview) is put into. Normally sometimes - especially when source email is sent from Microsoft Outlook - Emogrifier ends up with problem:
DOMXPath::query(): Invalid expression in .../Emogrifier.php:366

To avoid Emogrifier from parsing the email I have introduced in an email additional tag:

<message_preview><tr><td class="content-block"><?php echo $this->messageHtmlBody; ?></td></tr></message_preview>

Then after Emogrifier is initiated (.emogrify() method is called) I call function:
$emogrifier->addExcludedSelector('message_preview');

Unfortunatelly still the error persists.
The full code calling Emogrifier is:

$emogrifier = new Emogrifier(
            $this->serviceManager->get('ViewRenderer')->render($viewContent, null),
            file_get_contents($this->systemEmailsConfig['styles'])
        );
$emogrifier->addExcludedSelector('message_preview');
$this->mailContent = $emogrifier->emogrify();

As per my understanding this should prevent from using Emogrifier against anything that is inside <message_preview> tag - but apparently it does not.

@oliverklee oliverklee added the bug label Sep 19, 2016
@oliverklee oliverklee self-assigned this Sep 19, 2016
@oliverklee oliverklee added this to the 1.2.0 milestone Sep 19, 2016
@oliverklee
Copy link
Contributor

@voyteck0 Thanks for the bug report.

What exactly do you provide as $html to Emogrifier? The PHP code (i.e., including PHP opening and closing tags) or the output of executing PHP (i.e. only HTML, no PHP opening or closing tags)?

@oliverklee
Copy link
Contributor

FYI, excluding selectors does not keep Emogrifier from parsing that part as XML and then re-building it by outputting the XML - it only keeps the CSS from getting applied to it. If you need sections or your HTML to be completely untouched by Emogrifier, you need to put them into CDATA. (But maybe I've completely misunderstood your issue.)

@Voyteck
Copy link
Author

Voyteck commented Sep 19, 2016

Hi Oliver...
The code that is provided to Emogrifier is already rendered webpage - basing on template rendering from ZF2. So no PHP tags included - they are already replaced.

The problem is I believe that I am putting an email body into that. Imagine the system that is sending you an information any time email reaches specific account, while inside that notfication you have a body of that email. This is exactly waht I do. But since you have plenty different mail clients the way how the email (preview) is created is very different depedning on email (e.g. embedded pictures are treated differently in Outlook - they are attached to a mail as normal attachement with and referred in HTML as <img src=3D'cid:...'; Windows Mail - they are attached as normal attachment, but referred then in HTML as and GMail - they are attached to multipart MIME as inline and then referred in HTML as <img=3D'src=...'>).
Now the "raw" version of that HTML body is put into the file that is emogrified. And becouse of these various ways how mail clients works I wanted to exclude some part of an email from emogrifying - that is why I thought I can use that tag...

Can you elaborate more on how can I mark specific part of file not to be treated by Emogrifier ? The only workaround that comes to my mind is actually to put into file some string (e.g. &&&MESSAGE_PREVIEW&&&) that after emogrification will be simply replaced by code I want to replace it with. But maybe you could recommend better solution ?

Thanks in advance.

Wojtek

@oliverklee
Copy link
Contributor

Hi Wojtek,

could you please quote a minimal HTML and CSS that creates the problem for you? Thanks!

@Voyteck
Copy link
Author

Voyteck commented Sep 20, 2016

I could try - but fixing this on the issue level will not be an option.
As I experienced in my project various email clients treats standards in very various, I would say very agile way. That is why even once the problem is solved for this particular mail - on the next one it will come back again.
That is why what I actually needed (and thought it is working that way) is that I can put in a special tag content that will be ommited during emogrification process. I am currently working on implementing it on my solution - so I am removing everything inside special tag replacing it with a random hash, then emogrifying content, and after that reincluding the tag contents back where the hash appears.

So I think the bug should be rather recategorized for New Feature - but I would strongly recommend to update the instructions so they clearly states it only omits CSS inlining.

@oliverklee
Copy link
Contributor

I still don't quite understand what the exact issue is (i.e., what you are trying to do and what you mean by "ommited during emogrification process"). So the code would help me understand that.

@Voyteck
Copy link
Author

Voyteck commented Sep 20, 2016

Here's the full file that I send to emogrifier:
mailmessage.txt

Still if you look what is inside <message_preview> tag - there you can find "citated" message - and I think Emogrifier messes up there... But in general it happens (as per my current tests) only on Outlook emails - Windows Mail or GMail works fine. However I cannot guarantee that e.g. Apple mail will not make it fail again.
That is why what I was thinking of is simply making Emogrfier to ignore everything that is inside <message_preview> - so the email that is "attached" (included) in an email will not be processed (especially that it is not needed to process it - it is already processed before).

Additionally the following CSS code is added:

/* -------------------------------------
    GLOBAL
    A very basic CSS reset
------------------------------------- */
* {
  margin: 0;
  font-family: "Helvetica Neue", Helvetica, Arial, sans-serif;
  box-sizing: border-box;
  font-size: 14px;
}

img {
  max-width: 100%;
}

body {
  -webkit-font-smoothing: antialiased;
  -webkit-text-size-adjust: none;
  width: 100% !important;
  height: 100%;
  line-height: 1.6em;
  /* 1.6em * 14px = 22.4px, use px to get airier line-height also in Thunderbird, and Yahoo!, Outlook.com, AOL webmail clients */
  /*line-height: 22px;*/
}

/* Let's make sure all tables have defaults */
table td {
  vertical-align: top;
}

/* -------------------------------------
    BODY & CONTAINER
------------------------------------- */
body {
  background-color: #f6f6f6;
}

.body-wrap {
  background-color: #f6f6f6;
  width: 100%;
}

.container {
  display: block !important;
  max-width: 600px !important;
  margin: 0 auto !important;
  /* makes it centered */
  clear: both !important;
}

.content {
  max-width: 600px;
  margin: 0 auto;
  display: block;
  padding: 20px;
}

/* -------------------------------------
    HEADER, FOOTER, MAIN
------------------------------------- */
.main {
  background-color: #fff;
  border: 1px solid #e9e9e9;
  border-radius: 3px;
}

.content-wrap {
  padding: 20px;
}

.content-block {
  padding: 0 0 20px;
}

.header {
  width: 100%;
  margin-bottom: 20px;
}

.footer {
  width: 100%;
  clear: both;
  color: #999;
  padding: 20px;
}
.footer p, .footer a, .footer td {
  color: #999;
  font-size: 12px;
}

/* -------------------------------------
    TYPOGRAPHY
------------------------------------- */
h1, h2, h3 {
  font-family: "Helvetica Neue", Helvetica, Arial, "Lucida Grande", sans-serif;
  color: #000;
  margin: 40px 0 0;
  line-height: 1.2em;
  font-weight: 400;
}

h1 {
  font-size: 32px;
  font-weight: 500;
  /* 1.2em * 32px = 38.4px, use px to get airier line-height also in Thunderbird, and Yahoo!, Outlook.com, AOL webmail clients */
  /*line-height: 38px;*/
}

h2 {
  font-size: 24px;
  /* 1.2em * 24px = 28.8px, use px to get airier line-height also in Thunderbird, and Yahoo!, Outlook.com, AOL webmail clients */
  /*line-height: 29px;*/
}

h3 {
  font-size: 18px;
  /* 1.2em * 18px = 21.6px, use px to get airier line-height also in Thunderbird, and Yahoo!, Outlook.com, AOL webmail clients */
  /*line-height: 22px;*/
}

h4 {
  font-size: 14px;
  font-weight: 600;
}

p, ul, ol {
  margin-bottom: 10px;
  font-weight: normal;
}
p li, ul li, ol li {
  margin-left: 5px;
  list-style-position: inside;
}

/* -------------------------------------
    LINKS & BUTTONS
------------------------------------- */
a {
  color: #348eda;
  text-decoration: underline;
}

.btn-primary {
  text-decoration: none;
  color: #FFF;
  background-color: #348eda;
  border: solid #348eda;
  border-width: 10px 20px;
  line-height: 2em;
  /* 2em * 14px = 28px, use px to get airier line-height also in Thunderbird, and Yahoo!, Outlook.com, AOL webmail clients */
  /*line-height: 28px;*/
  font-weight: bold;
  text-align: center;
  cursor: pointer;
  display: inline-block;
  border-radius: 5px;
  text-transform: capitalize;
}

/* -------------------------------------
    OTHER STYLES THAT MIGHT BE USEFUL
------------------------------------- */
.last {
  margin-bottom: 0;
}

.first {
  margin-top: 0;
}

.aligncenter {
  text-align: center;
}

.alignright {
  text-align: right;
}

.alignleft {
  text-align: left;
}

.clear {
  clear: both;
}

/* -------------------------------------
    ALERTS
    Change the class depending on warning email, good email or bad email
------------------------------------- */
.alert {
  font-size: 16px;
  color: #fff;
  font-weight: 500;
  padding: 20px;
  text-align: center;
  border-radius: 3px 3px 0 0;
}
.alert a {
  color: #fff;
  text-decoration: none;
  font-weight: 500;
  font-size: 16px;
}
.alert.alert-warning {
  background-color: #FF9F00;
}
.alert.alert-bad {
  background-color: #D0021B;
}
.alert.alert-good {
  background-color: #68B90F;
}

/* -------------------------------------
    INVOICE
    Styles for the billing table
------------------------------------- */
.invoice {
  margin: 40px auto;
  text-align: left;
  width: 80%;
}
.invoice td {
  padding: 5px 0;
}
.invoice .invoice-items {
  width: 100%;
}
.invoice .invoice-items td {
  border-top: #eee 1px solid;
}
.invoice .invoice-items .total td {
  border-top: 2px solid #333;
  border-bottom: 2px solid #333;
  font-weight: 700;
}

/* -------------------------------------
    RESPONSIVE AND MOBILE FRIENDLY STYLES
------------------------------------- */
@media only screen and (max-width: 640px) {
  body {
    padding: 0 !important;
  }

  h1, h2, h3, h4 {
    font-weight: 800 !important;
    margin: 20px 0 5px !important;
  }

  h1 {
    font-size: 22px !important;
  }

  h2 {
    font-size: 18px !important;
  }

  h3 {
    font-size: 16px !important;
  }

  .container {
    padding: 0 !important;
    width: 100% !important;
  }

  .content {
    padding: 0 !important;
  }

  .content-wrap {
    padding: 10px !important;
  }

  .invoice {
    width: 100% !important;
  }
}

/*# sourceMappingURL=styles.css.map */

@oliverklee
Copy link
Contributor

Thanks for the code. I'll have a look at it.

Another question: You mentioned "when the email is sent from Microsoft Outlook". What is your workflow with Emogrifier? (I would assume that usually the email client is only used for reading the email, and that it the initial markup (template) is generated manually.)

@oliverklee
Copy link
Contributor

@voyteck0 In the meantime, would you be willing to provide a PR to make the workings of addExcludedSelector more explicit both in the method PHPDoc as well as in the README?

@Voyteck
Copy link
Author

Voyteck commented Sep 20, 2016

@oliverklee - the process my application is realizing, where Emogrifier is being used is following:

  1. John@hotmail.com composes email in MS Outlook to Michael@mydomain.com and sends it
  2. Email reaches mailserver.mydomain.com
  3. Email is qualified as one that should be quarantined and Steve@mydomain.com needs to be notified that such email has been sent to Michael@mydomain.com
  4. A confirmation message is created - includes some links and other content, but also a preview of email created by John@hotmail.com
  5. ...

Emogrifier comes in play in point 4 - I will go in more details there:
4.1. ZF2 is rendering a message using normal ViewRenderer and basing on HTML template + CSS file.
4.2. Inside the message a preview of email from John@hotmail.com is included - basically everything that is in HTML body + inline attachments (or the pictures that are embedded in the message - since some mail clients does not set properly Disposition header in MimePart)
4.3. The fully rendered HTML body of confirmation message (including the email preview) is passed to Emogrifier so all styles from CSS will be "inlined" - and here Emogrifier goes down :)

The solution is to tell Emgrifier which part of HTML it should simply not touch - and that is for I thought the addExcludedSelector() method can be used for.

Can you tell me what would you require from me in scope of "to make the workings of addExcludedSelector more explicit both in the method PHPDoc as well as in the README" - sorry, maybe I am not a specialist in GitHub, but do not understand that part :)

@oliverklee
Copy link
Contributor

@voyteck0 Do you need instructions on how to create a fork and a pull request for this, or do you need more explanations on the PHPDoc changes or the README changes?

@Voyteck
Copy link
Author

Voyteck commented Sep 21, 2016

I read a bit and found out what you mean... But I was actually not going to modify your class, but implement that "omitting" functionality in my code (somehow outside the class). I succeeded and please find the code that is realizing that below:

$mailContent = ... // here you need to put the string that will be emogrfied

        if (sizeof($ignoreEmogrifyingTags) > 0) {
            foreach($ignoreEmogrifyingTags as $tagToIgnore) {
                $ignoreReplacements[] = uniqid();
                $ignorePatterns[] = $pattern = "/<" . $tagToIgnore . ".*>(.*)<\/" . $tagToIgnore . ">/imsU";
                preg_match_all($pattern, $mailContent, $ignoreOriginals[]);
            }
            $mailContent = preg_replace($ignorePatterns, $ignoreReplacements, $mailContent);
            foreach($ignoreOriginals[0] as $key => $original)
                $ignoreOriginals[$key] = $original[0];
        }

        $emogrifier = new Emogrifier($mailContent, ...); // you need to add the CSS styles

        $mailContent = $emogrifier->emogrify();

        if (sizeof($ignoreEmogrifyingTags) > 0)
            $mailContent = str_replace($ignoreReplacements, $ignoreOriginals, $mailContent);

This is basically part of a function. Generally one of the parameters for the function is $ignoreEmogrifyingTags, which is an array (default array()) or strings. These strings are the tags that should be "left alone" once emogrifying - so they are basically excluded from the text (replaced by uniqid()) and after text is emogrified - they are pu back where they were originally.

I hope it will help if you'd wish to modify the way addExcludedSelector() method works.

@oliverklee
Copy link
Contributor

Thanks for the additional information! I'll have a look at it and then comment here with further ideas. Thanks again!

(And if you have anything else, please feel free to open another ticket. I'd also love it if we could get you on board as a contributor with a pull request. ❤️ )

@oliverklee oliverklee removed their assignment Jan 20, 2017
@oliverklee oliverklee modified the milestones: 1.3.0, 1.2.0 Mar 2, 2017
@oliverklee oliverklee modified the milestones: 1.3.0, 1.4.0 Oct 21, 2017
@oliverklee oliverklee modified the milestones: 2.1.0, 3.0.0 Apr 3, 2018
@JakeQZ
Copy link
Contributor

JakeQZ commented Sep 11, 2019

Looking at the code, I think addExcludedSelector() is behaving as intended, preventing nodes matching the selector from having inline styles applied. But to exclude a whole subtree would require a selector to match every element in the subtree.

So as well as

$emogrifier->addExcludedSelector('message_preview');

it would also be necessary to add an exclusion selector to match all the descendant elements:

$emogrifier->addExcludedSelector('message_preview *');

We should expand on the documentation and/or PHPDoc to clarify this. (I don't know whether, for convenience, a single selector list 'message_preview, message_preview *' would work with CssInliner - it probably won't with Emogrifier.)

We should also ensure that this use case is covered by unit tests.

@oliverklee
Copy link
Contributor

We should expand on the documentation and/or PHPDoc to clarify this.
[…]
We should also ensure that this use case is covered by unit tests.

I agree.

@JakeQZ JakeQZ removed the bug label Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants