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

[JakeQZ]

No description provided.

[JakeQZ]

I don't expect to have much time to follow up on this in the next few weeks, so this is just a "where I have got to" should anyone else want to take this on. It is the self-contained initial change to begin using sabberworm/php-css-parser that I referred to in #544. It should be broken down into smaller changesets to be committed to mainline individually, e.g. by starting with the changes to relax some testing contraints.

Frankly, I am finding some of the tools, particularly Psalm (but also PHPMD), to be more a PITA than offering any justifiable benefit. I would be more than happy to drop Psalm; it does not seem to be up to scratch to me, and generally hinders development because it is simply not smart enough, and lacks thorough (or even any) documentation.

[oliverklee]

I think we should keep using a type analysis tool. We could try using PHPStan instead of Psalm.

And in this case, I agree with PHPMD: The coupling is very high, and we should reduce it. Maybe we can add another class instead of stapling it all to CssInliner?

[JakeQZ]

I think we should keep using a type analysis tool. We could try using PHPStan instead of Psalm.

Let's stick with Psalm for now. Part of the problem was the pre-existing code using associative arrays instead of objects. If the use of Sabberworm is followed through, these would I hope be eliminated anyway.

However, it did seem that I had to follow a whole call stack chain telling Psalm more about the data that it should have been possible for it to determine itself. And I could not manage to get the array<…> docblock syntax to work with specifically-typed elements for specific keys, nor find any documentation on it - though from the errors reported by Psalm it seemed that it should be possible.

And in this case, I agree with PHPMD: The coupling is very high, and we should reduce it. Maybe we can add another class instead of stapling it all to CssInliner?

Perhaps some separate classes could be introduced, e.g. one for the return value of parseCssRules. However, Sabberworm has a large number of different object types for each type of CSS construct, so a significant number of use statements for each of these would be unavoidable (unless FQNs were used, but that would be defeating the point).

[oliverklee]

Let's stick with Psalm for now. Part of the problem was the pre-existing code using associative arrays instead of objects. If the use of Sabberworm is followed through, these would I hope be eliminated anyway.

Would #810 + friends still make sense to do first? Or does it make more sense I abandon my work on them?

[oliverklee]

Perhaps some separate classes could be introduced, e.g. one for the return value of parseCssRules.

Yes, that's the direction I was thinking of.

[oliverklee]

And maybe it would make sense to split up the CssInliner class more first. It still is way too big and too complex for my linking (and for PHPMD's liking).

[JakeQZ]

Would #810 + friends still make sense to do first? Or does it make more sense I abandon my work on them?

I think if we follow through the changes to use Sabberworm throughout we would or could end up replacing those new classes with Sabberworm classes, and could end up duplicating effort. It's not 100% clear to me at this stage, and when it will become clearer seems futher away now than it did a couple of weeks ago, but mostly because we have day jobs. (If this was our full time job, we could wrap this up in a couple of weeks, but I think more realistically we are looking at months, and several of them.) This PR itself may never be merged, but represents a vision of a direction we'd like to take (I say we, and I hope I can include you all in that).

So, I would say: please keep the work done on the branches as it may be useful/required; but don't spend any time on it for the time being (except as an academic exercise if you are bored), as it may become redundant.

And maybe it would make sense to split up the CssInliner class more first. It still is way too big and too complex for my linking (and for PHPMD's liking).

I think this would be a good line of attack. We would need to think carefully about how it should be split up. There is already some functionality in there that is duplicated in sister classes; that could be a starting point (see #734, #738 and #739 - whether it should be in a trait or separate class is not written in stone). Other than that, at this stage, all I can say is "OOD" - think about what the objects are, and see if that suggests a route for refactoring...? :)

Which sort of brings us back full-circle into some kind of philosophical dilemma:

• We can't introduce the library until we refactor the code;
• The only way to refactor the code is to duplicate what the library does.

Hmm...

[oliverklee]

I will switch the default git branch from master to main in a minute. GitHub then will autoclose all PRs targeted at the removed/renamed branch. Please create a new PR (from the existing PR branch) targeting main.