-
Notifications
You must be signed in to change notification settings - Fork 154
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] Fix PhpStorm code inspection warnings #770
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks generally fine, but there are a couple of issues - in particular with regard maintaining support for the Emogrifier
class without an autoloader.
src/Emogrifier.php
Outdated
@@ -843,7 +843,7 @@ private function normalizeStyleAttributesOfAllNodes() | |||
private function normalizeStyleAttributes(\DOMElement $node) | |||
{ | |||
$normalizedOriginalStyle = \preg_replace_callback( | |||
'/[A-z\\-]+(?=\\:)/S', | |||
'/[a-zA-Z\\-]+(?=:)/S', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch - it happened to work more by luck because :
is before A
in ASCII.
However, I don't think the correction is quite right - AKAIK property names may also contain underscores or numbers (though I'm not aware of any), if they are classed as CSS identifiers - the regex used in HtmlPruner::removeRedundantClassesAfterCssInlined
matches a CSS identifier (after a period).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased, changed accordingly and repushed.
src/Emogrifier/CssInliner.php
Outdated
@@ -380,7 +380,7 @@ private function getAllNodesWithStyleAttribute() | |||
private function normalizeStyleAttributes(\DOMElement $node) | |||
{ | |||
$normalizedOriginalStyle = \preg_replace_callback( | |||
'/[A-z\\-]+(?=\\:)/S', | |||
'/[a-zA-Z\\-]+(?=:)/S', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible issue as with the same regex in Emogrifier
.
The autloader issue is a non-issue, but do you think the regex for property names should be updated to better match the spec? |
761ed69
to
290c84b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Fixes #729