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

Remove Emogrifier (temporary) #14899

Closed
wants to merge 1 commit into from

Conversation

JoshuaLuckers
Copy link
Contributor

What does it do?

Remove Emogrifier (temporary)

Why is it needed?

Emogrifier's platform requirements prevent us from installing MODX using composer install.
Travis also fails due to this. Once we can use a version of Emogrifier that allows us to install it under PHP 7.4 we can add it back.

Related issue(s)/PR(s)

@JoshuaLuckers JoshuaLuckers added area-core pr/review-needed Pull request requires review and testing. labels Dec 14, 2019
@opengeek
Copy link
Member

I would honestly like to see if we can find an alternative to this project. Even their current solution is going to specifically add 7.4 to their list of platform dependencies. There are no other dependencies we are using that I am aware of which do this. That means when 7.5 comes out, we will go through the same problem again.

If there is no other viable solution, then perhaps we can add it back. But in the meantime, I'd like to try to find a replacement.

@Mark-H
Copy link
Collaborator

Mark-H commented Dec 16, 2019

We could add an event into modPHPMailer (something like OnBeforeMailSend or whatever) and move the functionality of inlining CSS to a separate extra.

@opengeek
Copy link
Member

I like that idea. Would it have to be inside the PHPMailer implementation of modMail, or could it be generic enough to work with all modMail implementations?

@bezumkin
Copy link
Contributor

@Mark-H original idea was to make MODX default emails pretty, so I made this template with styles.

If you will remove or move Emogrifier - you also will need to rewrite this template.

@Mark-H
Copy link
Collaborator

Mark-H commented Dec 16, 2019

Don't you think we can keep the nice template, and just accept that it may not look perfect in all email clients, unless people install an extra to inline the CSS? I've not checked recently, but hopefully the more modern clients will also parse some styles from the head?

I'm very happy the default templates got their much needed upgrade, great work on that. :)

@Mark-H
Copy link
Collaborator

Mark-H commented Dec 16, 2019

I like that idea. Would it have to be inside the PHPMailer implementation of modMail, or could it be generic enough to work with all modMail implementations? @opengeek

It looks like it could probably also go into modMail.send and use the get/set methods on modMail to get/set the body. That should then work PHPMailer, but not with modSwiftMailer (the only third party mailer I know off) because that does not call the parent::send method.

As the latter does not look like it'll work in 3.0 without updates regardless, I'd be happy to only focus on the core modMail/modPHPMailer.

@opengeek
Copy link
Member

@Mark-H Yeah, as long as it is possible to use the event with any implementation, that's all I wanted to accomplish. It looks like modSwiftMailer might not be maintained any longer and I may look into updating it to see if I can get it to work with the proposed event.

@bezumkin
Copy link
Contributor

@Mark-H

I've not checked recently, but hopefully the more modern clients will also parse some styles from the head?

Gmail still not modern enough to support styles head tag.

I've made a new PR to replace Emogrifier to InlineStyle. Looks like it makes the same job but much more lightweight - only one class and pretty simple composer.json

@opengeek
Copy link
Member

I opted for using #14905 in order to move forward on this issue. We can continue the discussion though.

@opengeek opengeek closed this Dec 18, 2019
@JoshuaLuckers JoshuaLuckers deleted the remove-emogrifier branch January 11, 2020 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core pr/review-needed Pull request requires review and testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants