Failing tests with CSS Parser 8.4

[JakeQZ]

E.g. https://github.com/MyIntervals/emogrifier/runs/4501517317

There were 2 failures:

1) Pelago\Emogrifier\Tests\Unit\Css\CssDocumentTest::parsesSelector with data set "attribute (value match)" ('[href=https://example.org]')
Failed asserting that actual size 0 matches expected size 1.

/home/runner/work/emogrifier/emogrifier/tests/Unit/Css/CssDocumentTest.php:40

2) Pelago\Emogrifer\Tests\Unit\CssInlinerTest::inlineCssInDebugModeForInvalidCssSelectorThrowsException
Failed asserting that exception of type "Symfony\Component\CssSelector\Exception\SyntaxErrorException" is thrown.

Looks like something has changed in Symfony that we need to update for - the tests don't fail with "lowest" dependencies.

(Not sure why I've only just got the linked build report for a commit several days ago nor why it wasn't picked up by CI tests.)

[JakeQZ]

(Not sure why I've only just got the linked build report for a commit several days ago nor why it wasn't picked up by CI tests.)

Oh, we have some kind of weekly build, and it seems to be labelled based on the last commit rather than anything more useful.

I think it's just coincidence that two 3rd-party libs have just been updated in a way which breaks our CI build.

[oliverklee]

The first failure might be this bug: symfony/symfony#44516

[JakeQZ]

I confirmed these result from using CSS Parser 8.4 and pass with 8.3.1.

The second is because the invalid rule is now stripped out by CSS Parser so it never reaches Symfony for it to throw an exception. Will need to look into what behaviour/debug settings are avaible so we can catch the exception.

[JakeQZ]

The other looks very closely related but is a bug introduced into CSS Parser. With the new version, the valid CSS rule [href=https://example.org]{ color: green; } is now being discarded.

I'm guessing a change has been made to discard genuininely invalid CSS selectors more in line with the spec, however in this case there is a false positive.

[JakeQZ]

is a bug

...unless I'm mistaken and [href=https://example.org] is actually an invalid selector, but AFAIK there is no requirement to quote the value in this case.

[JakeQZ]

There's a 'lenient parsing' option but that is true by default and hasn't changed between 8.3.1 and 8.4. I also can't see anything obvious in the changelog suggesting the change in behaviour.

[JakeQZ]

MyIntervals/PHP-CSS-Parser#162 appears to be the culprit, the commit MyIntervals/PHP-CSS-Parser@2c5cd6f

[JakeQZ]

I think how to address the bug in the CSS Parser perhaps needs some discussion with a focus on longer term. Maybe it is possible to fix the regex here, I haven't looked in detail.

Regarding the exception no longer being thrown, this may be more of a discussion issue, but one I'd still be keen to see a 'moving-forward' outcome on; any changes to the CSS Parser I think would be classed as a feature request. Probably, most simply, optionally throwing an exception on invalid data, as we do in 'debug' mode (we could pass on the 'debug' mode flag to CSS Parser) would solve this (I see an excpetion appears to be thrown but is then caught later).

[JakeQZ]

The second is because the invalid rule is now stripped out by CSS Parser so it never reaches Symfony for it to throw an exception. Will need to look into what behaviour/debug settings are avaible so we can catch the exception.

We can chain on our debug setting to \Sabberworm\CSS\Settings::withLenientParsing() (inversely - debug equates to don't parse leniently but instead throw exceptions).

However, this is not immediately straightforward as doing so causes other of our tests to fail. Those failures need to be looked into. It is possibly mostly due to delibrate creation of malformed CSS for the tests from the time we had our own rudimentary CSS parser. This relates to the discussion in #1135 regarding tests that may be reduntant or in the wrong place.

[oliverklee]

Our test is wrong, and we should remove it altogether:
MyIntervals/PHP-CSS-Parser#347 (comment)
https://codepen.io/raxbg/pen/OJZgRVz

[JakeQZ]

Our test is wrong, and we should remove it altogether

Actually we should change the tests to use quoted attribute values in CSS selectors. The are quite a few others which use unquoted values, but don't fail, presumably because CSS Parser is still allowing these due to not containing special characters like : and /.

[oliverklee]

We've bumped to PHP-CSS-Parser 8.7, and the tests are green. So this does not seem to be an issue anymore. Closing.