-
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
Ignore invalid selectors, see #179 #180
Conversation
Hi @Synchro, thanks for the PR! Could you please add a regression test (i.e., a minimal unit test that fails without the fix and that passes with the fix)? Could you also please prefix the commit message subject with [BUGFIX](and, if you have more than one commit, squash those commits)? Thanks! |
@@ -262,6 +262,10 @@ public function emogrify() | |||
// query the body for the xpath selector | |||
$nodesMatchingCssSelectors = $xpath->query($this->translateCssToXpath($value['selector'])); | |||
|
|||
// Ignore invalid selectors | |||
if (!$nodesMatchingCssSelectors) { |
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 is not always a boolean. So we should not treat it as a boolean, but do an explicit === false instead.
Done all those things, rebased, pushed. The test case was a little tricky - needed to create an error handler to trap the warnings. I've arranged it so that it eats the first error type, but lets the second go through. That can be removed when you need a test case for the other error. |
This is a partial fix for #179. It simply adds a check on the xpath query return value before attempting to iterate over the result set. If the xpath has failed, it just issues a continue to go to the next iteration.
This fixes the second of the two errors mentioned in the ticket, so it's effectively treating the symptom rather than the cause, but it's better than nothing! I don't know how it would be best to try to prevent the first error - somehow validate the CSS selector fragment before trying to pass it to the xpath converter?