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

Update lib\Pelago\Emogrifier (install w/ composer) #2340

Closed
wants to merge 3 commits into from

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Jul 26, 2022

Description (*)

@fballiano tried to install this with composer, but it didnt work because of php version conflicts or it does not fit to OM code anymore. Updated OM code to work with latest releases of Pelago\Emogrifier.

  • v4 => PHP 7.0 support
  • v5 => PHP 7.1
  • v6 => up to PHP 8.1

Related Pull Requests

See #2138

Manual testing scenarios (*)

  1. Preview newsletter template from admin section

Questions or comments

Todo: add to upcoming release builder?

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@sreichel sreichel added the PHP 8 Related to PHP8 label Jul 26, 2022
@github-actions github-actions bot added Component: Core Relates to Mage_Core composer Relates to composer.json labels Jul 26, 2022
@github-actions github-actions bot added the Component: lib/* Relates to lib/* label Jul 26, 2022
@github-actions

This comment has been minimized.

1 similar comment
@github-actions
Copy link
Contributor

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit 7d0c5e1. ± Comparison against base commit 6eaea99.

@OpenMage OpenMage deleted a comment from github-actions bot Jul 26, 2022
@fballiano
Copy link
Contributor

precisely what I did here: https://github.com/fballiano/openmage/blob/mcommerce/app/code/core/Mage/Core/Model/Email/Template/Abstract.php so I like this PR.

I like the removal of the composer.lock, it makes everything difficult :-)

Problem, if we move this library to composer then we need the release builder #2165 or we can't do a release.

@sreichel
Copy link
Contributor Author

@fballiano disableInlineStyleAttributesParsing() or disableStyleBlocksParsing()?

@fballiano
Copy link
Contributor

Reading the documentation here: https://github.com/MyIntervals/emogrifier:

disableStyleBlocksParsing() - By default, CssInliner will grab all <style> blocks in the HTML and will apply the CSS styles as inline "style" attributes to the HTML. The <style> blocks will then be removed from the HTML. If you want to disable this functionality so that CssInliner leaves these <style> blocks in the HTML and does not parse them, you should use this option. If you use this option, the contents of the <style> blocks will not be applied as inline styles and any CSS you want CssInliner to use must be passed in as described in the Usage section above.

I think disableStyleBlocksParsing shouldn't be called in openmage, the style blocks should be parsed.

For disableInlineStyleAttributesParsing (that I had in my code) instead:

disableInlineStyleAttributesParsing() - By default, CssInliner preserves all of the "style" attributes on tags in the HTML you pass to it. However if you want to discard all existing inline styles in the HTML before the CSS is applied, you should use this option.

I don't really understand the documentation, I think inline styles should be left as they are no?

@fballiano
Copy link
Contributor

I think the disableStyleBlocksParsing should be removed, <style> blocks should be parsed by the library.

About disableInlineStyleAttributesParsing, checking the source code used I would leave it as default (not calling the disableInlineStyleAttributesParsing method)

@sreichel sreichel marked this pull request as draft August 13, 2022 21:53
@sreichel sreichel closed this Sep 27, 2022
@sreichel
Copy link
Contributor Author

See #2411

@sreichel sreichel deleted the update-Pelago_Emogrifier branch September 27, 2022 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Core Relates to Mage_Core Component: lib/* Relates to lib/* composer Relates to composer.json PHP 8 Related to PHP8
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants