[FEATURE] Add support for inserting an item in a CSSList

[ziegenberg]

This is based on and will supersede #115. Suggestions from the original PR are incorporated, and the initial authorship is preserved.

[ziegenberg]

I fixed the Static Analysis errors. Do I need to fix the unit test? They are working with PHP 8.

[oliverklee]

I fixed the Static Analysis errors.

Thanks! 🙏

Do I need to fix the unit test? They are working with PHP 8.

Yes, please. AFAICT, the mixed type annotation is the culprit, as it's only available in PHP 8 and up, not in PHP 7.x: https://www.php.net/manual/en/language.types.mixed.php

[ziegenberg]

Fixed it.

[oliverklee]

Thanks a bunch for this contribution! ❤️

I have added a few comments - one concerning the architecture, and a few concerning minor style nits.

[oliverklee]

The tested method has two kinds of behavior: replacing and inserting. Please let's have two different tests for this.

The test name should be a statement communicating the tested behavior. This allows the test name and test code to form a kind of "double bookkeeping". Please have a look at this slide about naming tests: https://speakerdeck.com/oliverklee/test-driven-development-with-phpunit-915a5f2c-3d37-4e41-b5bc-ef1efca9c01e?slide=39

[ziegenberg]

I tend to dissagree. As the method name suggests, we insert an item. Yes, it does this by internally mapping to the existing replace() function, but in the end, for the consumer the item is inserted and not replaced. It does append (inserts at the end, before the not found sibbling), if it can't find the spot to insert the item, but this two different insertion points are reflected in the naming of the test data in the dataprovider.

Splitting this in to two test methods basiaclly doublicates the code, because the test code would be exactly the same.

But if you insist, I will make the change.

[oliverklee]

Yes, please. (I find some duplication in tests okay as long as it helps them tell an easy-to read story.)

[oliverklee]

We probably then also can do without the data providers and instead include the data directly in the corresponding tests.

[ziegenberg]

I separated the two tests, as requested. Happy now?

[ziegenberg]

But where's the point of having data providers if you separate everything into its own testing method? This is now a lot of duplicate code. Take e.g. expandBorderShorthand() and its expandBorderShorthandProvider() in tests/RuleSet/DeclarationBlockTest.php. Shouldn't this rather be:

• expandBorderShorthandExpandsBorderWithBorderStyleAndBorderColor
• expandBorderShorthandExpandsBorderStyle
• expandBorderShorthandExpandsBorderWidth
• expandBorderShorthandExpandsBorderColor
• expandBorderShorthandExpandsBorderWithBorderStyle
• expandBorderShorthandFixesWhitespace

following the same approach as done here?

[oliverklee]

For testing expandBorderShorthand, I'd differentiate between two pieces of behavior:

1. that is expands different border things (I'd use a data provider for that)
2. that it fixes whitespace

So concerning splitting tests, I'd differentiate between "the same behavior, but with different data" and different behavior.

Let's have a non-code example - maybe this will help me communicate my thinking. :-)

• different behavior: I can eat. I can draw. I can spit things out.
• same behavior, but with different data (data providers): I can eat apples, bananas and bread.

Does this help, @ziegenberg?

Would it be helpful to have a call to talk about testing strategies and patterns?

[ziegenberg]

I rebased onto the current main, while I was at it.

[oliverklee]

This feature also deserves a changelog entry.

[ziegenberg]

TLDR: Is there anything left open that needs addressing?

IMHO, this PR can be considered thoroughly reviewed in 53 comments, with about ten commits for one mere conditional and two function calls in one new method (not counting the testing code). I started this because since 2016, Moodle has been dragging PR #115 with every update to the PHP-CSS-Parser lib, and I wanted to fix this properly.

[JakeQZ]

This LGTM. I agree there is a bit of duplication in setting up the test subject, but we could argue until the cows come home about the best way to avoid that duplication.

IMO, let's get this in as it is now, since it's critical path for other projects, then possibly review the test code duplication later. (There's also #390 needed for an 8.6 release.)

[oliverklee]

Looks good to me. Thanks! 🙏