Fix InvalidArgumentException #392
Conversation
…r (eg: css3 keyframes).
|
HI @jriboux, thanks a lot for the PR! Could you please add a unit test that fails without the change and that passes with it? And could you also please add a line to the changelog file? |
Classes/Emogrifier.php
Outdated
| $nodesMatchingCssSelectors = $xPath->query($this->translateCssToXpath($cssRule['selector'])); | ||
| } catch (\InvalidArgumentException $e) { | ||
| $nodesMatchingCssSelectors = false; | ||
| } // ignore invalid selectors |
There was a problem hiding this comment.
Please move this comment to above the catch (in its own line).
|
Sorry, my changes are in conflict with pull request #361 . Could we implement a debug switch, false by default that would :
|
|
Yes, a debug switch would be good. It should be off by default, and we should have a method public setDebug(bool $debug): void. |
CHANGELOG.md
Outdated
| @@ -7,7 +7,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). | |||
| ## x.y.z (unreleased) | |||
|
|
|||
| ### Added | |||
|
|
|||
| - Debug mode. Throw debug exceptions only if debug is active. | |||
There was a problem hiding this comment.
Could you also please link the PR (i.e., use the same format as for the other changelog entries)?
Classes/Emogrifier.php
Outdated
| @@ -226,6 +226,13 @@ class Emogrifier | |||
| ]; | |||
|
|
|||
| /** | |||
| * If true, allow debug Exception to be thrown. | |||
There was a problem hiding this comment.
Let's make this more specific: … Emogrifier will throw Exceptions when it encounters an error instead of silently ignoring them.
Classes/Emogrifier.php
Outdated
| $line | ||
| ) | ||
| ); | ||
| if ($this->debug) { |
There was a problem hiding this comment.
We don't need the else here as throwing an exception will exit the method anyway.
Classes/Emogrifier.php
Outdated
| } | ||
|
|
||
| // the normal error handling continues when handler return false | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Set debug mode. |
There was a problem hiding this comment.
Please always use the third person for the first sentence of a method description: [This Method] "Sets the debug mode."
Classes/Emogrifier.php
Outdated
| * Set debug mode. | ||
| * | ||
| * @param bool $debug set to true to enable debug mode | ||
| */ |
There was a problem hiding this comment.
Please also add the return void annotation.
Tests/Unit/EmogrifierTest.php
Outdated
| $this->subject->emogrify(); | ||
| } | ||
|
|
||
| /** | ||
| * @test | ||
| */ | ||
| public function emogrifyIgnoreInvalidCssSelectors() |
There was a problem hiding this comment.
Please also add another test that tests for the behavior without debug mode.
There was a problem hiding this comment.
To test with debug mode enabled I kept the test from pull request #361 "Handling invalid xPath expression warnings" and added the following line to test debug mode :
$this->subject->setDebug(true);
See lines 948 to 961
The test emogrifyIgnoreInvalidCssSelectors it the test with debug mode disabled.
|
Merged. Thanks for contributing to Emogrifier, @jriboux! ❤️ |
Fix InvalidArgumentException when xpath does not find the css selector (eg: css3 keyframes).
You can reproduce the InvalidArgumentException using the following css :