[BUGFIX] Handle invalid/unrecognized selectors in media query blocks #442
Conversation
…yIntervals#441) - Throw `\InvalidArgumentException` for invalid/unrecognized selectors in debug mode; - Keep media query blocks containing invalid/non-recognized selectors when not in debug mode.
| if ($this->debug) { | ||
| throw $e; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
This should be false, if I'm not mistaken.
There was a problem hiding this comment.
It may happen that a CSS selector is valid, but is not recognized (not implemented) by Emogrifier. This is why I thought it would be good to keep all selectors we can't figure out whether they are invalid for real or just not (yet) implemented.
Maybe there should be a switch/option (keep unrecognized selectors or not)?
There was a problem hiding this comment.
There is no way for Emogrifier to "keep" a selector if it cannot properly match it. Either Emogrifier can match a selector (then it gets converted to an xPath selector, and the CSS will get applied to the HTML), or it cannot match it (then the CSS will not be applied).
There was a problem hiding this comment.
As far as I can understand the code, this keeping/not keeping procedure is applied (only) for media query blocks: if at least one of the selectors from a media query block has matching nodes (is relevant for the document), the whole media query block is kept in the final output. See method copyCssWithMediaToStyleNode.
Of course it would make sense to not keep whole media query blocks, but only declarations with relevant selectors, but that's another PR.
There was a problem hiding this comment.
I've read this method again: The return value of the method (technically) indicates this:
return $nodesMatchingSelector !== false && $nodesMatchingSelector->length !== 0;This means that it should return true if the selector has found at least one node and has not created any errors. So I think the return value should be false for errors in non-debug mode.
Does this make sense, or am I still missing anything?
There was a problem hiding this comment.
Please consider the following example (as reported in #439, slightly modified):
@media only screen and (min-device-width: 768px) and (max-device-width: 1024px) {
.mobile_link a[href^="tel"] {
color: blue;
}
}
This would throw a \RuntimeException with current master, an \InvalidArgumentException in debug mode with this PR. But the CSS selector is actually a correct one, it is just not recognized be Emogrifier yet (see issue #443). So returning false would eventually eliminate the above media query block from the end result, which may not be the good decision.
Unfortunately, in case of unrecognized selectors, we do not know whether they are actually invalid or just not implemented, so just to be sure, I think we should trust the input and keep unrecognized CSS selectors in media query blocks, not dump them. This is why a put return true in non-debug mode.
There was a problem hiding this comment.
Ah, thanks for the explanation. Makes sense.
| /** | ||
| * @test | ||
| */ | ||
| public function emogrifyKeepsInvalidOrUnrecognizedSelectorsInMediaQueryBlocksInNonDebugMode() |
Solves issue #441:
\InvalidArgumentExceptionfor invalid/unrecognized selectors indebug mode;
not in debug mode.