-
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] Silence purposefully ignored PHP warnings #400
[BUGFIX] Silence purposefully ignored PHP warnings #400
Conversation
Hi @MarkCarver, thanks for the PR! I'll have a look at it. |
It looks like we'll also need to migrate the Travis job so it can run for this PR. I'm on it. |
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.
@MarkCarver The code changes look very good, thanks!
Could you please adapt the comments (see my review comments for details) and check that the code sniffer also approves, and then re-push?
vendor/bin/phpcs --standard=Configuration/PhpCodeSniffer/Standards/Emogrifier/ Classes/ Tests/
Thanks! 🍪
Classes/Emogrifier.php
Outdated
// Instead, Emogrifier's error handler should always throw an exception and it must be caught here and only rethrown if in debug mode. | ||
try { | ||
// query the body for the xpath selector | ||
$nodesMatchingCssSelectors = $xPath->query($this->translateCssToXpath($cssRule['selector'])); |
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.
I think we should have a comment here (directly above the line with the query() call) explaining that this will never be false as we already handle this with via the error handler.
Classes/Emogrifier.php
Outdated
$nodesMatchingCssSelectors = $xPath->query($this->translateCssToXpath($cssRule['selector'])); | ||
} | ||
catch (\InvalidArgumentException $e) { | ||
// Rethrow exception if in debug mode. |
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.
This comment (and also the next one) are not necessary as the code already is "speaking" enough, and not having the (redundant) comment will allow us to not have to maintain it. :-)
Classes/Emogrifier.php
Outdated
// Unfortunately, this would only apply to tests and not work for production executions, which can still flood logs/output unnecessarily. | ||
// Instead, Emogrifier's error handler should always throw an exception and it must be caught here and only rethrown if in debug mode. | ||
try { | ||
// query the body for the xpath selector |
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.
This comment also is not needed as the code already is "speaking" enough.
Classes/Emogrifier.php
Outdated
continue; | ||
// There's no real way to test "PHP Warning" output generated by the following XPath query unless PHPUnit converts it to an exception. | ||
// Unfortunately, this would only apply to tests and not work for production executions, which can still flood logs/output unnecessarily. | ||
// Instead, Emogrifier's error handler should always throw an exception and it must be caught here and only rethrown if in debug mode. |
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.
Great comment!
@@ -28,6 +28,8 @@ This project adheres to [Semantic Versioning](http://semver.org/). | |||
|
|||
|
|||
### Fixed | |||
- Silence purposefully ignored PHP Warnings | |||
([#400](https://github.com/MyIntervals/emogrifier/pull/400)) |
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.
Thanks!
Classes/Emogrifier.php
Outdated
@@ -1535,7 +1545,8 @@ private function getNodesToExclude(\DOMXPath $xPath) | |||
*/ | |||
public function handleXpathError($type, $message, $file, $line, array $context) | |||
{ | |||
if ($this->debug && $type === E_WARNING && isset($context['cssRule']['selector'])) { | |||
// Do not tie this conditional to debug mode as it causes unnecessary output. See further explanation in ::process() where this exception is caught. |
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.
👍
Done |
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!
Merged. Thanks for contributing, @MarkCarver! ❤️ |
NOTE: I know this doesn't have tests because this kind of logic isn't really "testable" in the first place other than what already exists.
Instead, I have verbosely commented why the error handler must always throw an exception and not be tied to the debug option.
This fixes #361 and #392 which introduced this new error handler and started spitting out PHP Warnings when it's ultimately just ignored anyway.