Save @import tag #334

frangolet · 2016-09-01T13:53:17Z

It removes @import tag from css.
Example:

<style type="text/css">
 @import url('https://fonts.googleapis.com/css?family=Open+Sans:400,400italic,700,700italic');
 @media{ body, table, td, p, a, li, blockquote {-webkit-text-size-adjust:none !important;}  }
</style>

------------->>>>

<style type="text/css">
 @media{ body, table, td, p, a, li, blockquote {-webkit-text-size-adjust:none !important;}  }
</style>

<<<<------------

It removes @import tag from css. Example: >>>>----------- <style type="text/css"> @import url('https://fonts.googleapis.com/css?family=Open+Sans:400,400italic,700,700italic'); @media{ body, table, td, p, a, li, blockquote {-webkit-text-size-adjust:none !important;} } </style> ------------->>>> <style type="text/css"> @media{ body, table, td, p, a, li, blockquote {-webkit-text-size-adjust:none !important;} } </style> <<<<------------

oliverklee · 2016-09-05T13:28:05Z

Classes/Emogrifier.php

@@ -911,13 +911,14 @@ private function attributeValueIsImportant($attributeValue)
     *
     * @return void
     */
-    private function copyCssWithMediaToStyleNode(\DOMDocument $xmlDocument, \DOMXPath $xPath, $css)
+    private function copyCssWithMediaToStyleNode(\DOMDocument $xmlDocument, \DOMXPath $xPath, $css, $imports = '')


This pull request does not change the calls of this method, i.e, $imports will always be an empty string. So there will be no change in behavior. Or am I missing something?

And why does the new parameter have a default value? Do you expects (at least) two different calls (one with the argument and one without it)?

Hi, I did removed the default value for imports and added another missing line so the imports are passed now to that function.

oliverklee · 2016-09-05T13:29:29Z

Hi @asfwebmaster, thanks for the PR!

It still needs some more love, though. Could you please reformat the code to be PSR-2 compliant? And could you please add unit tests that fail without your changes and pass with them? Thanks!

oliverklee · 2016-09-05T13:30:58Z

Classes/Emogrifier.php

+		$imports = '';
+		preg_match_all('/^\\s*@import\\s[^;]+;/misU', $css, $importsMatches, PREG_PATTERN_ORDER);
+		if( !empty($importsMatches[0]) ){
+			$importsArray = reset($importsMatches);


You don't need a reset before doing an implode.

reset was giving me the first element of the array, same as $importsMatches[0] so I did remove the reset and replaced with $importsMatches[0]. Can you give me some more info what is not PSR-2. Thanks in advanced man.

oliverklee · 2016-09-06T10:48:50Z

Classes/Emogrifier.php

@@ -911,13 +911,14 @@ private function attributeValueIsImportant($attributeValue)
     *
     * @return void
     */
-    private function copyCssWithMediaToStyleNode(\DOMDocument $xmlDocument, \DOMXPath $xPath, $css)
+    private function copyCssWithMediaToStyleNode(\DOMDocument $xmlDocument, \DOMXPath $xPath, $css, $imports)


Please add the missing param annotation.

oliverklee · 2016-09-06T16:36:42Z

Now that I think of it, I'm not sure that loading externals font files from an within e-mail is a good idea in the first place. On desktop email clients like Thunderbird, the loading of external resources is disabled by default (i.e., the font will not be loaded), and on smartphones, you wouldn't want to eat up the bandwidth for relatively cosmetic things like web fonts in emails. I opt for continuing to remove the imports. @jjriv What do you think?

andrispraulitis · 2016-09-07T13:00:02Z

@oliverklee Thunderbird is just one of the email clients that do not support link tag for custom fonts but does support @import. I don't see why this should not be allowed. It should really be my choice if I want to include custom fonts and eat bandwidth.

jjriv · 2016-09-07T17:57:32Z

Anyone sending emails for marketing is likely going to want to use web fonts at some point. Campaign Monitor supports this functionality. I would argue that we should support this if email sending companies also support it, since Emogrifier is an alternative to CM and should remain competitive. And Emogrifier could be used to build such an email sending service.

Here is the article from Campaign Monitor:
https://www.campaignmonitor.com/resources/guides/web-fonts-in-email/

It is also embraced by Litmus:
https://litmus.com/blog/typography-tips-for-email-designers

And MailChimp:
http://kb.mailchimp.com/campaigns/design/limitations-of-html-email

oliverklee · 2016-09-07T22:00:36Z

@jjriv Thanks for the info! Then let's have this.

@asfwebmaster Can you please update your PR to fix the PSR-2 issues and also add tests that fail without your change and pass with it (and also adapt the existing tests as needed)?

Also, can you please add a changelog entry (newest on top) and squash your commits (as explained in the CONTRIBUTING file)? Thanks!

Hi, I did it in different way now. Added two functions, one to extract the imports and one to apply them to a style node. I think this way is more clear. And not messing up with splitCssAndMediaQuery function. Please have a look and let me know what you think. Thank you in advance.

oliverklee · 2016-09-09T15:44:10Z

Classes/Emogrifier.php

@@ -350,7 +350,8 @@ protected function process(\DOMDocument $xmlDocument)
        if ($this->isStyleBlocksParsingEnabled) {
            $allCss .= $this->getCssFromAllStyleNodes($xPath);
        }
-
+
+		$cssImports = $this->extractImportsFromCss($allCss);


Can you please read over all changes and fix the indentation? Thanks!

believe me I m trying to, the git editor not saving it correct

On 9 Sep 2016, at 16:44, Oliver Klee notifications@github.com wrote:

indentation

oliverklee · 2016-09-09T16:09:58Z

Classes/Emogrifier.php

@@ -394,6 +395,7 @@ protected function process(\DOMDocument $xmlDocument)
            $this->removeInvisibleNodes($xPath);
        }

+	$this->copyImportsToStyleNode($xmlDocument, $xPath, $cssImports);


Please use 4 spaces for indentation, not tabs or 8 spaces. You can find the PSR-2 style guide here:
https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-coding-style-guide.md

I highly recommend that you read it. It's not very long, and it's used by a lot of projects.

oliverklee · 2016-09-09T16:11:58Z

@asfwebmaster I recommend you read up on "git commit --amend" and on squashing commits. Otherwise (if you wait and add more commits to this PR), squashing commits gets harder the more commits you have.

frangolet · 2016-09-09T16:36:17Z

Hi Man, Can you please help me with this issue. I m not very good with git. I did all this changes on github.com the problem is that when I change the tabs to 4 in their editor nothing is changed, or is changed but not as should be. If you can edit it somehow would be great if not can you please give me a suggestion how to do it? Thank you in advance.

On 9 Sep 2016, at 17:11, Oliver Klee notifications@github.com wrote:

git commit --amend

oliverklee · 2016-09-09T16:54:42Z

I'm glad to help.

First of all, are you using an IDE (like PhpStorm) for doing the Git stuff, or are you doing it on the command line? I highly recommend doing this on the command line as you then see what you are doing (whereas an IDE will hide the stuff "under the hood" and might also do things differently).

On the command line, you can do git status to see which files are changed. If you change the indentation, the changed file should be listed. Is it listed?

And if you are really new to Git on the command line, I highly recommend the free course "Try Git" on CodeSchool: https://www.codeschool.com/courses/try-git

oliverklee · 2016-09-09T16:56:33Z

Oh, I just read that you did all changes in the web interface of GitHub. This does not work well at all. You absolutely need to check out your fork and work on a local copy to get a clean PR. Please have a look at the CONTRIBUTING document - I've linked some helpful guides from it.

oliverklee · 2016-09-09T16:57:47Z

This guide explains the thing with forks and PRs: https://gist.github.com/Chaser324/ce0505fbed06b947d962

Signed-off-by: Alex Frangulev <alex@kwtdesign.co.uk>

oliverklee · 2016-09-10T11:39:20Z

Oh, and you absolutely need to have a local copy to work on so you can run the tests locally.

Signed-off-by: Alex Frangulev <alex@kwtdesign.co.uk>

The proposed solution in #334 for retaining `@import` rules has a flaw: it may keep such rules that were commented-out. By separating out the comment removal from other CSS parsing, we can avoid that pitfall. Also optimized the regex for CSS comments by unfolding and using possessive quantifiers, potentially avoiding catastrophic failure. Part of #338.

oliverklee reviewed Sep 5, 2016
View reviewed changes

oliverklee added the waiting for feedback label Sep 5, 2016

oliverklee reviewed Sep 5, 2016
View reviewed changes

oliverklee added this to the 1.2.0 milestone Sep 5, 2016

frangolet added 2 commits September 5, 2016 14:43

Update Emogrifier.php

b8eb532

Update Emogrifier.php

8e7a4d3

oliverklee self-assigned this Sep 5, 2016

oliverklee reviewed Sep 6, 2016
View reviewed changes

oliverklee mentioned this pull request Sep 7, 2016

@import gets removed from style tag #331

Closed

oliverklee reviewed Sep 9, 2016
View reviewed changes

frangolet added 3 commits September 9, 2016 16:55

Update Emogrifier.php

3178fed

Update Emogrifier.php

e444b58

Update Emogrifier.php

fd11a13

oliverklee reviewed Sep 9, 2016
View reviewed changes

fix tabs

1e88c7a

Signed-off-by: Alex Frangulev <alex@kwtdesign.co.uk>

Alex Frangulev added 2 commits September 12, 2016 10:43

fix crlf

e85c95a

Signed-off-by: Alex Frangulev <alex@kwtdesign.co.uk>

PHP-CS-Fixer PSR-2

43d4fe6

Signed-off-by: Alex Frangulev <alex@kwtdesign.co.uk>

frangolet closed this Sep 12, 2016

frangolet deleted the patch-1 branch September 12, 2016 10:25

oliverklee mentioned this pull request Sep 12, 2016

Support @import #338

Closed

oliverklee removed the waiting for feedback label Mar 1, 2017

JakeQZ mentioned this pull request Sep 28, 2019

[CLEANUP] Move CSS comment removal to separate method #728

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save @import tag #334

Save @import tag #334

frangolet commented Sep 1, 2016 •

edited

Loading

oliverklee Sep 5, 2016

oliverklee Sep 5, 2016

frangolet Sep 5, 2016

oliverklee commented Sep 5, 2016

oliverklee Sep 5, 2016

frangolet Sep 5, 2016

oliverklee Sep 6, 2016

oliverklee commented Sep 6, 2016

andrispraulitis commented Sep 7, 2016 •

edited

Loading

jjriv commented Sep 7, 2016

oliverklee commented Sep 7, 2016

oliverklee Sep 9, 2016

frangolet Sep 9, 2016

oliverklee Sep 9, 2016

oliverklee commented Sep 9, 2016

frangolet commented Sep 9, 2016

oliverklee commented Sep 9, 2016

oliverklee commented Sep 9, 2016

oliverklee commented Sep 9, 2016

oliverklee commented Sep 10, 2016

Save @import tag #334

Save @import tag #334

Conversation

frangolet commented Sep 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverklee commented Sep 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverklee commented Sep 6, 2016

andrispraulitis commented Sep 7, 2016 • edited Loading

jjriv commented Sep 7, 2016

oliverklee commented Sep 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverklee commented Sep 9, 2016

frangolet commented Sep 9, 2016

oliverklee commented Sep 9, 2016

oliverklee commented Sep 9, 2016

oliverklee commented Sep 9, 2016

oliverklee commented Sep 10, 2016

frangolet commented Sep 1, 2016 •

edited

Loading

andrispraulitis commented Sep 7, 2016 •

edited

Loading