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

Feature/32702 #247

Merged
merged 15 commits into from
Dec 18, 2018
Merged

Feature/32702 #247

merged 15 commits into from
Dec 18, 2018

Conversation

Januszpl
Copy link
Contributor

Pelago/emogrifier is used for transforming email-inline css into inline css inside email content that later is send to the customer by Magento. It has some limitation related to selector and special character and @charset "UTF-8"; is one of the problem. However not sure if my solution is the best - maybe some "if" to remove it only for flagged files like email* ?

…: Invalid expression invendor/pelago/emogrifier/Classes/Emogrifier.php on line 269 by remoqinv @charset UTF_8
@Januszpl Januszpl requested a review from Igloczek August 29, 2017 13:03
@Januszpl Januszpl changed the base branch from master to develop August 29, 2017 13:03
@Igloczek Igloczek self-assigned this Sep 29, 2017
@Igloczek
Copy link
Contributor

Igloczek commented Oct 9, 2017

I think we should ask for changes in the mentioned M2 dependency, b/c replacing this in this way is not so elegant and didn't fix the issue for everyone.

Related issue: MyIntervals/emogrifier#296

Copy link
Contributor

@Igloczek Igloczek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's limit this just to email-inline file and I'm okay to merge it.

@Stevie-Ray
Copy link

As a workaround we created a Composer patch:

patches/composer/github-issue-emogrifier-296.diff

 ### Fixed
+- Allow `@charset` in the CSS without error (note: its value is currently
+  ignored, [#507](https://github.com/MyIntervals/emogrifier/pull/507))
 - Allow attribute selectors in descendants
   ([#506](https://github.com/MyIntervals/emogrifier/pull/506),
   [#381](https://github.com/MyIntervals/emogrifier/issues/381))
diff --git a/Classes/Emogrifier.php b/Classes/Emogrifier.php
index f227ada..538e060 100644
--- a/Classes/Emogrifier.php
+++ b/Classes/Emogrifier.php
@@ -1252,7 +1252,7 @@ function ($matches) use (&$media) {

         // filter the CSS
         $search = [
-            'import directives' => '/^\\s*@import\\s[^;]+;/misU',
+            'import/charset directives' => '/^\\s*@(?:import|charset)\\s[^;]+;/misU',
             'remaining media enclosures' => '/^\\s*@media\\s[^{]+{(.*)}\\s*}\\s/misU',
         ];

composer.json

"extra": {
        "magento-force": "override",
        "composer-exit-on-patch-failure": true,
        "patches": {
            "pelago/emogrifier": {
                "Support @charset in the CSS #296": "patches/composer/github-issue-emogrifier-296.diff"
            }
        }
    }

@Januszpl
Copy link
Contributor Author

@Igloczek what do you think? I think better to make as you said - just limit this replace to one css file

@Stevie-Ray
Copy link

Magento Open Source 2.3.0 Release Notes

Emogrifier dependency has been updated to ^2.0.0. Fix submitted by Oliver Klee in pull request 13351.

@Januszpl
Copy link
Contributor Author

@Stevie-Ray great. However I will still add extra task next week for email-inline.css only so if someone would need it (like previous M2 versions etc) he can use it.

@Januszpl
Copy link
Contributor Author

Januszpl commented Dec 7, 2018

@Igloczek tested
gulp styles and email inbox before and after running gulp email-fix
https://i.gyazo.com/8bc1ca044d935aaa458612627b9981d4.png

@Igloczek Igloczek merged commit aff98db into develop Dec 18, 2018
@Igloczek Igloczek deleted the feature/32702 branch December 18, 2018 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants