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

[BUGFIX] Allow @charset without error #507

Merged
merged 2 commits into from
Feb 8, 2018

Conversation

JakeQZ
Copy link
Contributor

@JakeQZ JakeQZ commented Feb 8, 2018

Filter out @charset at-rules from the CSS before processing, to avoid an error
when presented with CSS containing this at-rule.

Obviously this does not mean @charset is supported - its value is still
ignored - but does address the immediate error in #296.

Added PHPUnit test like that for @import, and also test that such CSS content
does not prevent Emogrifier working correctly on the remainder of the CSS.

Filter out `@charset` at-rules from the CSS before processing, to avoid an error
when presented with CSS containing this at-rule.

Obviously this does not mean `@charset` is supported - its value is still
ignored - but does address the immediate error in MyIntervals#296.

Added PHPUnit test like that for `@import`, and also test that such CSS content
does not prevent Emogrifier working correctly on the remainder of the CSS.
@oliverklee oliverklee self-requested a review February 8, 2018 18:00
@oliverklee oliverklee added the bug label Feb 8, 2018
@oliverklee oliverklee added this to the 2.1.0 milestone Feb 8, 2018
Copy link
Contributor

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

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

Look generally very good. The test name needs more love, though.

*
* @dataProvider unneededCssThingsDataProvider
*/
public function emogrifyUnaffectedByUnneededCssThings($css)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please let's rename the test to be more specific. What does "unaffected" mean?

(And please add a verb so that the test name is a (possible pidgin) sentence.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have renamed the method.

Copy link
Contributor

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

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

Beautiful!

@oliverklee oliverklee merged commit 806d28c into MyIntervals:master Feb 8, 2018
@oliverklee
Copy link
Contributor

Merged. Thanks for the bug fix, @JakeQZ! ❤️

@JakeQZ JakeQZ deleted the at-charset-error-fix branch February 14, 2018 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants