[TASK] Use sabberworm/php-css-parser to parse the CSS #1015

JakeQZ · 2021-04-13T23:42:39Z

No description provided.

oliverklee · 2021-04-18T11:42:27Z

sabberworm/php-css-parser has not been very well-maintained lately. We might need to help out there (or choose a different library).

JakeQZ · 2021-04-18T23:58:31Z

sabberworm/php-css-parser has not been very well-maintained lately. We might need to help out there (or choose a different library).

Last release was 10 months ago. Last commit was 5 months ago. But I'm not aware of any new CSS syntaxes being introduced, so there may be no pressing reason to update anything particularly regularly. Of course new PHP 7 or Psalm-compatible type hints would be nice.

Inevitably the new tests are failing since rebase due to CSS whitespace differences (and @import "foo.css" being rendered as @import url("foo.css").

oliverklee · 2021-04-19T09:08:40Z

I'm mostly worried about situations where we need a particular bug fix or a compatibility fix for newer PHP versions. (And I'm hopeful that we can contribute to the project, and that the maintainer will accept our changes.)

oliverklee · 2021-04-27T09:15:58Z

In the meantime, it looks like the maintainers of the Sabberworm CSS parser review and merge new PRs (slowly but steadily). We might need to take care of updating old PRs and fixing old bugs ourselves, though (including bugs reported by Nikita Popov from the PHP core team).

bytestream · 2021-05-24T11:47:05Z

I think adding Sabberworm CSS parser is a good idea. With respect to other libraries, the only other one I'm aware of is https://github.com/Cerdic/CSSTidy but I think you'll find similar issues with this one.

JakeQZ · 2021-05-25T15:30:54Z

With respect to other libraries, the only other one I'm aware of is https://github.com/Cerdic/CSSTidy

Thanks, I was not aware of that library. I think it would certainly be worth looking into before committing more heavily to the other one - e.g. starting to use its classes further downstream (which this PR doesn't go as far as).

This is currently a demonstration to see the code changes and potential test failures involved with using this CSS parsing package rather than `sabberworm/php-css-parser` as proposed in #1015.

JakeQZ · 2021-05-26T00:17:35Z

https://github.com/Cerdic/CSSTidy

I think it would certainly be worth looking into before committing more heavily to the other one.

I should have been getting on with something else, but thought it worth checking out how CSSTidy might pan out if we were to decide to go with that instead. I've created a draft PR, #1076, for that alternative, so you can see what the code changes look like and what tests would have to be modified or disabled.

The advantages of one vs the other appear to be as follows:

CSSTidy

Seems possibly able to combine shorthand and longhand properties;
Has potentially revealed a bug in our code.

Sabberworm

Supports PHP 8;
Provides classes we could use downstream;
Rejects invalid (malformed) property names;
Allows invalid @import rules to be rejected.

Coincidentally, this PR itself is now ready for review, should we decide to proceed with the Sabberworm parser...

src/CssInliner.php

tests/Unit/CssInlinerTest.php

JakeQZ · 2021-05-26T01:05:44Z

Coincidentally, this PR itself is now ready for review, should we decide to proceed with the Sabberworm parser...

Marked as draft again, pending #1077.

This is currently a demonstration to see the code changes and potential test failures involved with using this CSS parsing package rather than `sabberworm/php-css-parser` as proposed in #1015.

JakeQZ · 2021-05-26T14:58:06Z

Marked as draft again, pending #1077.

Now rebased with #1077. This is ready for review, in conjunction with the possible alternative in #1076.

This is currently a demonstration to see the code changes and potential test failures involved with using this CSS parsing package rather than `sabberworm/php-css-parser` as proposed in #1015.

oliverklee

I've added my thoughts on reading through the first version.

src/Utilities/CssDocument.php

oliverklee · 2021-05-28T17:09:20Z

src/Utilities/CssDocument.php

+                        continue;
+                    }
+                }
+                $media = '@media ' . $mediaQueryList;


I'm not a big fan of having the types in variable names. Maybe we can rename the variable to $mediaQueries?

According to the spec, or at least MDN, it is quite specifically called a "media query list". Changing it to something else I think could sow more confusion.

src/Utilities/CssDocument.php

oliverklee · 2021-05-28T17:11:14Z

src/Utilities/CssDocument.php

+                    && $rule->getRules('font-family') !== []
+                    && $rule->getRules('src') !== [];
+            default:
+                return true;


Please let's have early returns only for guard clauses.

I think there can be a case for breaking this rule, if all of the following apply:

The swtich statement is the only statement in the method;

All case branches have a return statment and not a break statement, except where a 'follow-through' is allowed or for the last case or default if null or void is the obviosuly desired return value in that case.

However, maintaining that standard through future code changes may be harder than it seems. So I will probably change it when I revisit the code changes on this - perhaps tomorrow.

if all of the following apply

Such a method would be atomic. If there was any future need to add pre-amble or post-amble code, then it should be moved to a new atomic method, with the original method becoming a wrapper with the additional pre- or post- code. Though we know from experience it often doesn't turn out that way.

Please let's have early returns only for guard clauses.

I've changed the switch statement, though not the preceding code.

I don't think I agree with a "one size fits all" approach regarding where a return statement is or isn't acceptable, provided the complexity of an individual method can be kept low, as I've indicated above.

src/Utilities/CssDocument.php

Also restore the `preg_quote`.

JakeQZ · 2021-06-11T01:03:47Z

I've hopefully now resolved all the code review issues, and rebased to include #1081. Unfortunately the latter has uncovered a bug in the Sabberworm CSS parser which is beyond the scope of this PR, so it will still not be as clean a commit as desired, because some newly-added tests must now be temporarily disabled.

oliverklee

Looks very good! Now we only need the final polish.

src/Utilities/CssDocument.php

JakeQZ · 2021-06-12T01:09:23Z

Looks very good! Now we only need the final polish.

Have made suggested changes.

Do I get the prize of not having to make a rebase involving a file rename? ;))

JakeQZ · 2021-06-12T01:26:56Z

... rebased to include #1081. Unfortunately the latter has uncovered a bug in the Sabberworm CSS parser which is beyond the scope of this PR, so it will still not be as clean a commit as desired, because some newly-added tests must now be temporarily disabled.

MyIntervals/PHP-CSS-Parser#261

oliverklee

Beautiful!

JakeQZ added enhancement cleanup labels Apr 13, 2021

JakeQZ self-assigned this Apr 13, 2021

JakeQZ marked this pull request as draft April 13, 2021 23:42

JakeQZ mentioned this pull request Apr 13, 2021

[CLEANUP] Move CSS parsing to a separate class #1014

Merged

JakeQZ force-pushed the task/sabberworm-css-parser branch 5 times, most recently from fdcc73e to f51de35 Compare April 17, 2021 23:41

JakeQZ force-pushed the task/sabberworm-css-parser branch from f51de35 to df8f1a6 Compare April 18, 2021 23:47

JakeQZ force-pushed the task/sabberworm-css-parser branch from df8f1a6 to 15c4a7a Compare April 19, 2021 00:10

JakeQZ force-pushed the task/sabberworm-css-parser branch from 15c4a7a to 5a326c0 Compare April 26, 2021 23:47

JakeQZ force-pushed the task/sabberworm-css-parser branch from 5a326c0 to e8d4f16 Compare May 4, 2021 23:10

JakeQZ force-pushed the task/sabberworm-css-parser branch 2 times, most recently from 139680f to c73a134 Compare May 23, 2021 22:48

JakeQZ force-pushed the task/sabberworm-css-parser branch from c73a134 to 7a4226f Compare May 25, 2021 21:36

JakeQZ mentioned this pull request May 25, 2021

[TASK] Use cerdic/css-tidy to parse the CSS #1076

Closed

JakeQZ force-pushed the task/sabberworm-css-parser branch from 7a4226f to 112a73d Compare May 26, 2021 00:02

JakeQZ added this to the 6.0.0 milestone May 26, 2021

JakeQZ requested a review from oliverklee May 26, 2021 00:02

JakeQZ marked this pull request as ready for review May 26, 2021 00:17

JakeQZ added the discussion label May 26, 2021

JakeQZ force-pushed the task/sabberworm-css-parser branch from 112a73d to 3eda043 Compare May 26, 2021 00:20

JakeQZ commented May 26, 2021

View reviewed changes

src/CssInliner.php Outdated Show resolved Hide resolved

JakeQZ commented May 26, 2021

View reviewed changes

tests/Unit/CssInlinerTest.php Outdated Show resolved Hide resolved

JakeQZ marked this pull request as draft May 26, 2021 01:05

JakeQZ force-pushed the task/sabberworm-css-parser branch from 3eda043 to b84f00c Compare May 26, 2021 14:54

JakeQZ marked this pull request as ready for review May 26, 2021 14:58

oliverklee requested changes May 28, 2021

View reviewed changes

oliverklee requested changes May 29, 2021

View reviewed changes

src/Utilities/CssDocument.php Outdated Show resolved Hide resolved

src/Utilities/CssDocument.php Outdated Show resolved Hide resolved

src/Utilities/CssDocument.php Show resolved Hide resolved

src/Utilities/CssDocument.php Outdated Show resolved Hide resolved

JakeQZ mentioned this pull request Jun 10, 2021

[CLEANUP] Refactor into a new StyleRule class #1082

Merged

[TASK] Use sabberworm/php-css-parser to parse the CSS

fb7a7f3

JakeQZ force-pushed the task/sabberworm-css-parser branch from b84f00c to fb7a7f3 Compare June 10, 2021 23:33

JakeQZ added 4 commits June 11, 2021 00:40

Disable tests that fail due to Sabberworm bug

c81396f

Move @media rule filtering to separate method

cb365b6

Also restore the `preg_quote`.

Make callable static and add type hints

066825c

Don't return directly from switch statement

cbf581f

oliverklee requested changes Jun 11, 2021

View reviewed changes

Further changes suggested in review

858db98

oliverklee approved these changes Jun 12, 2021

View reviewed changes

oliverklee merged commit c2ad938 into main Jun 12, 2021

oliverklee deleted the task/sabberworm-css-parser branch June 12, 2021 07:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TASK] Use sabberworm/php-css-parser to parse the CSS #1015

[TASK] Use sabberworm/php-css-parser to parse the CSS #1015

JakeQZ commented Apr 13, 2021 •

edited

oliverklee commented Apr 18, 2021

JakeQZ commented Apr 18, 2021

oliverklee commented Apr 19, 2021

oliverklee commented Apr 27, 2021

bytestream commented May 24, 2021

JakeQZ commented May 25, 2021

JakeQZ commented May 26, 2021 •

edited

JakeQZ commented May 26, 2021

JakeQZ commented May 26, 2021

oliverklee left a comment

oliverklee May 28, 2021

JakeQZ Jun 11, 2021

oliverklee May 28, 2021

JakeQZ May 30, 2021

JakeQZ May 30, 2021

JakeQZ Jun 11, 2021

JakeQZ commented Jun 11, 2021

oliverklee left a comment

JakeQZ commented Jun 12, 2021

JakeQZ commented Jun 12, 2021

oliverklee left a comment

[TASK] Use sabberworm/php-css-parser to parse the CSS #1015

[TASK] Use sabberworm/php-css-parser to parse the CSS #1015

Conversation

JakeQZ commented Apr 13, 2021 • edited

oliverklee commented Apr 18, 2021

JakeQZ commented Apr 18, 2021

oliverklee commented Apr 19, 2021

oliverklee commented Apr 27, 2021

bytestream commented May 24, 2021

JakeQZ commented May 25, 2021

JakeQZ commented May 26, 2021 • edited

JakeQZ commented May 26, 2021

JakeQZ commented May 26, 2021

oliverklee left a comment

Choose a reason for hiding this comment

oliverklee May 28, 2021

Choose a reason for hiding this comment

JakeQZ Jun 11, 2021

Choose a reason for hiding this comment

oliverklee May 28, 2021

Choose a reason for hiding this comment

JakeQZ May 30, 2021

Choose a reason for hiding this comment

JakeQZ May 30, 2021

Choose a reason for hiding this comment

JakeQZ Jun 11, 2021

Choose a reason for hiding this comment

JakeQZ commented Jun 11, 2021

oliverklee left a comment

Choose a reason for hiding this comment

JakeQZ commented Jun 12, 2021

JakeQZ commented Jun 12, 2021

oliverklee left a comment

Choose a reason for hiding this comment

JakeQZ commented Apr 13, 2021 •

edited

JakeQZ commented May 26, 2021 •

edited