From 5bf2e1487f5fb3d1d5cbd55068985c31f1c9b500 Mon Sep 17 00:00:00 2001
From: JakeQZ <jake@qzdesign.co.uk>
Date: Thu, 11 Jun 2020 11:11:29 +0100
Subject: [PATCH] [BUGFIX] Avoid exception with :last-of-type etc. (#875)

This simply broadens the range of exceptions thrown by `symfony/css-selector`
(for unsupported selector expressions) which will be caught and discarded in
non-debug mode.

It does not make anything magically work which didn't previously; however, it
does allow the rest of the CSS to achieve full functionality, with only the
affected CSS rule and selector suffering.

The tests and changes to the README clarify and correct-as-documented the
current behavior for these edge cases, which could be improved upon in future.

These are currently rarely-used CSS pseudo-classes; their level of support in
mainstream browsers has only
[recently reached 95% of audiences](https://caniuse.com/#search=last-of-type).
Their level of support in email clients is likely to be drastically lower per
se of some time to come, but may be something Emogrifier can help with moving
forwards, as their use becomes more widespread...

Closes #872.
New issue #876 for follow-up.
---
 CHANGELOG.md                  |   2 +
 README.md                     |  19 +++---
 src/CssInliner.php            |  16 ++---
 tests/Unit/CssInlinerTest.php | 115 ++++++++++++++++++++++++++++++++++
 4 files changed, 136 insertions(+), 16 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 61d472cb..30e3da7c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -73,6 +73,8 @@ This project adheres to [Semantic Versioning](https://semver.org/).
   ([#773](https://github.com/MyIntervals/emogrifier/pull/773))
 
 ### Fixed
+- Allow `:last-of-type` etc. without type, without causing exception
+  ([#875](https://github.com/MyIntervals/emogrifier/pull/875))
 - Make sure to use the Composer-installed development tools
   ([#862](https://github.com/MyIntervals/emogrifier/pull/862),
   [#865](https://github.com/MyIntervals/emogrifier/pull/865))
diff --git a/README.md b/README.md
index effab2a8..c217ac60 100644
--- a/README.md
+++ b/README.md
@@ -308,33 +308,36 @@ Emogrifier currently supports the following
    * [first-child](https://developer.mozilla.org/en-US/docs/Web/CSS/:first-child)
    * [first-of-type](https://developer.mozilla.org/en-US/docs/Web/CSS/:first-of-type)
      (with a type, e.g. `p:first-of-type` but not `*:first-of-type` which will
-     behave as `*:first-child`)
+     currently be treated as `*:not(*)`)
    * [last-child](https://developer.mozilla.org/en-US/docs/Web/CSS/:last-child)
    * [last-of-type](https://developer.mozilla.org/en-US/docs/Web/CSS/:last-of-type)
-     (with a type)
+     (with a type &ndash; without a type, it will be treated as `:not(*)`)
    * [not()](https://developer.mozilla.org/en-US/docs/Web/CSS/:not)
    * [nth-child()](https://developer.mozilla.org/en-US/docs/Web/CSS/:nth-child)
    * [nth-last-child()](https://developer.mozilla.org/en-US/docs/Web/CSS/:nth-last-child)
    * [nth-last-of-type()](https://developer.mozilla.org/en-US/docs/Web/CSS/:nth-last-of-type)
-     (with a type)
+     (with a type &ndash; without a type, it will be treated as `:not(*)`)
    * [nth-of-type()](https://developer.mozilla.org/en-US/docs/Web/CSS/:nth-of-type)
-     (with a type)
+     (with a type &ndash; without a type, it will be applied as if `:nth-child`)
    * [only-child](https://developer.mozilla.org/en-US/docs/Web/CSS/:only-child)
    * [only-of-type](https://developer.mozilla.org/en-US/docs/Web/CSS/:only-of-type)
-     (with a type)
+     (with a type &ndash; without a type, it will be applied as if `:only-child`
+     or `:not(*)`, depending on version constraints for `symfony/css-selector`)
 
 The following selectors are not implemented yet:
 
  * [case-insensitive attribute value](https://developer.mozilla.org/en-US/docs/Web/CSS/Attribute_selectors#case-insensitive)
  * static [pseudo-classes](https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-classes):
    * [first-of-type](https://developer.mozilla.org/en-US/docs/Web/CSS/:first-of-type)
-     without a type (will behave as `:first-child`)
+     without a type (declarations discarded)
    * [last-of-type](https://developer.mozilla.org/en-US/docs/Web/CSS/:last-of-type)
-     without a type (will behave as `:last-child`)
+     without a type (declarations discarded)
    * [nth-last-of-type()](https://developer.mozilla.org/en-US/docs/Web/CSS/:nth-last-of-type)
-     without a type (will behave as `:nth-last-child()`)
+     without a type (declarations discarded)
    * [nth-of-type()](https://developer.mozilla.org/en-US/docs/Web/CSS/:nth-of-type)
      without a type (will behave as `:nth-child()`)
+   * [only-of-type()](https://developer.mozilla.org/en-US/docs/Web/CSS/:only-of-type)
+     without a type (will behave as `:only-child()` or `:not(*)`)
    * any pseudo-classes not listed above as supported – rules involving them
      will nonetheless be preserved and copied to a `<style>` element in the 
      HTML – including (but not necessarily limited to) the following:
diff --git a/src/CssInliner.php b/src/CssInliner.php
index 8757793d..30dd635c 100644
--- a/src/CssInliner.php
+++ b/src/CssInliner.php
@@ -7,7 +7,7 @@
 use Pelago\Emogrifier\HtmlProcessor\AbstractHtmlProcessor;
 use Pelago\Emogrifier\Utilities\CssConcatenator;
 use Symfony\Component\CssSelector\CssSelectorConverter;
-use Symfony\Component\CssSelector\Exception\SyntaxErrorException;
+use Symfony\Component\CssSelector\Exception\ParseException;
 
 /**
  * This class provides functions for converting CSS styles into inline style attributes in your HTML code.
@@ -153,7 +153,7 @@ class CssInliner extends AbstractHtmlProcessor
      *
      * @return self fluent interface
      *
-     * @throws SyntaxErrorException
+     * @throws ParseException
      */
     public function inlineCss(string $css = ''): self
     {
@@ -183,7 +183,7 @@ public function inlineCss(string $css = ''): self
         foreach ($cssRules['inlinable'] as $cssRule) {
             try {
                 $nodesMatchingCssSelectors = $this->xPath->query($cssSelectorConverter->toXPath($cssRule['selector']));
-            } catch (SyntaxErrorException $e) {
+            } catch (ParseException $e) {
                 if ($this->debug) {
                     throw $e;
                 }
@@ -569,7 +569,7 @@ private function extractFontFaceRules(string $css): array
      *
      * @return \DOMElement[]
      *
-     * @throws SyntaxErrorException
+     * @throws ParseException
      */
     private function getNodesToExclude(): array
     {
@@ -577,7 +577,7 @@ private function getNodesToExclude(): array
         foreach (\array_keys($this->excludedSelectors) as $selectorToExclude) {
             try {
                 $matchingNodes = $this->xPath->query($this->getCssSelectorConverter()->toXPath($selectorToExclude));
-            } catch (SyntaxErrorException $e) {
+            } catch (ParseException $e) {
                 if ($this->debug) {
                     throw $e;
                 }
@@ -1018,7 +1018,7 @@ private function determineMatchingUninlinableCssRules(array $cssRules)
      *
      * @return bool
      *
-     * @throws SyntaxErrorException
+     * @throws ParseException
      */
     private function existsMatchForSelectorInCssRule(array $cssRule): bool
     {
@@ -1038,13 +1038,13 @@ private function existsMatchForSelectorInCssRule(array $cssRule): bool
      *
      * @return bool
      *
-     * @throws SyntaxErrorException
+     * @throws ParseException
      */
     private function existsMatchForCssSelector(string $cssSelector): bool
     {
         try {
             $nodesMatchingSelector = $this->xPath->query($this->getCssSelectorConverter()->toXPath($cssSelector));
-        } catch (SyntaxErrorException $e) {
+        } catch (ParseException $e) {
             if ($this->debug) {
                 throw $e;
             }
diff --git a/tests/Unit/CssInlinerTest.php b/tests/Unit/CssInlinerTest.php
index d74ee587..dc75953d 100644
--- a/tests/Unit/CssInlinerTest.php
+++ b/tests/Unit/CssInlinerTest.php
@@ -3083,6 +3083,121 @@ public function inlineCssNotInDebugModeKeepsInvalidOrUnrecognizedSelectorsInMedi
         self::assertContainsCss($css, $subject->render());
     }
 
+    /**
+     * @return string[][]
+     */
+    public function provideCssRulesWithUnsupportedSelectorCombination(): array
+    {
+        return [
+            ':first-of-type without type' => [':first-of-type { color: red; }'],
+            ':last-of-type without type' => [':last-of-type { color: red; }'],
+            ':nth-last-of-type without type' => [':nth-last-of-type(2n) { color: red; }'],
+        ];
+    }
+
+    /**
+     * This test enstablishes the current expected/observed behaviour with currently supported versions of
+     * `symfony/css-selector` for static pseudo-classes which are only partially supported.
+     *
+     * The handling of these selectors should be revisited - rules with unsupported combinations should be copied to a
+     * <style> element so that they can at least be applied correctly by fully-supporting email clients.  It is also
+     * possible that (before then) future changes to Symfony may break this test, in which case the documentation would
+     * need updating and the tests adjusting.
+     *
+     * @test
+     *
+     * @param string $css
+     *
+     * @dataProvider provideCssRulesWithUnsupportedSelectorCombination
+     */
+    public function inlineCssNotInDebugModeDiscardsRulesWithUnsupportedSelectorCombination(string $css)
+    {
+        $subject = CssInliner::fromHtml('<html><p>Hello</p><p>World</p></html>');
+        $subject->setDebug(false);
+
+        $subject->inlineCss($css);
+
+        $result = $subject->render();
+        self::assertNotContainsCss($css, $result);
+        self::assertNotContains('color: red', $result);
+    }
+
+    /**
+     * @return string[][]
+     */
+    public function provideCssRulesWithPossiblyIncorrectlyImplementedSelectorCombination(): array
+    {
+        return [
+            ':only-of-type without type' => [':only-of-type { color: red; }'],
+        ];
+    }
+
+    /**
+     * This test enstablishes the current expected/observed behaviour with currently supported versions of
+     * `symfony/css-selector` for static pseudo-classes which are only partially supported.
+     *
+     * The handling of these selectors should be revisited - rules with unsupported combinations should be copied to a
+     * <style> element so that they can at least be applied correctly by fully-supporting email clients.  It is also
+     * possible that (before then) future changes to Symfony may break this test, in which case the documentation would
+     * need updating and the tests adjusting.
+     *
+     * @test
+     *
+     * @param string $css
+     *
+     * @dataProvider provideCssRulesWithPossiblyIncorrectlyImplementedSelectorCombination
+     */
+    public function inlineCssNotInDebugModeMayDiscardRulesWithPossiblyIncorrectlyImplementedSelectorCombination(
+        string $css
+    ) {
+        $subject = CssInliner::fromHtml('<html><p>Hello</p><p>World</p></html>');
+        $subject->setDebug(false);
+
+        $subject->inlineCss($css);
+
+        $result = $subject->render();
+        self::assertNotContainsCss($css, $result);
+        // The declaration may or may not be haphazardly applied depending on `symofony/css-selector` version.
+        // Nothing more can be asserted that would always be true for the full range of versions supported.
+    }
+
+    /**
+     * @return string[][]
+     */
+    public function provideCssRulesWithIncorrectlyImplementedSelectorCombination(): array
+    {
+        return [
+            ':nth-of-type without type' => [':nth-of-type(2n) { color: red; }'],
+        ];
+    }
+
+    /**
+     * This test enstablishes the current expected/observed behaviour with currently supported versions of
+     * `symfony/css-selector` for static pseudo-classes which are only partially supported.
+     *
+     * The handling of these selectors should be revisited - rules with unsupported combinations should be copied to a
+     * <style> element so that they can at least be applied correctly by fully-supporting email clients.  It is also
+     * possible that (before then) future changes to Symfony may break this test, in which case the documentation would
+     * need updating and the tests adjusting.
+     *
+     * @test
+     *
+     * @param string $css
+     *
+     * @dataProvider provideCssRulesWithIncorrectlyImplementedSelectorCombination
+     */
+    public function inlineCssAppliesRulesWithIncorrectlyImplementedSelectorCombination(string $css)
+    {
+        $subject = $this->buildDebugSubject('<html><p>Hello</p><p>World</p></html>');
+
+        $subject->inlineCss($css);
+
+        $result = $subject->render();
+        self::assertNotContainsCss($css, $result);
+        // The declaration will currently be haphazardly applied: in the case of `...of-type`, as if `...child`.
+        self::assertContains('color: red', $result);
+    }
+
     /**
      * @return string[][]
      */