[TASK] Deprecate sSpaceAfterListArgumentSeparator being an array

[oliverklee]

No description provided.

[JakeQZ]

Wow. I couldn't think of a way of achieving the seemingly impossible (well, not impossible but it looked like a lot of work). But you just have. Very clever. (Do you play chess?)

But there's a premature change here, I think.

[JakeQZ]

Too soon. We need to still allow array until 9.0.

[JakeQZ]

Oops, was looking at the diff the wrong way round. But...

Can we use @deprecated in the DocBlock here for partial deprecation?

[JakeQZ]

But there's a premature change here, I think.

I was mistaken about that, but wonder if it's possible to use @deprecated in the DocBlock for a partial deprecation...

[oliverklee]

I was mistaken about that, but wonder if it's possible to use @deprecated in the DocBlock for a partial deprecation...

As far as I know, that's not possible. @deprecated annotations always refer to one "thing" in its entirety, be it class, method, property or parameter.

[oliverklee]

Wow. I couldn't think of a way of achieving the seemingly impossible (well, not impossible but it looked like a lot of work). But you just have. Very clever. (Do you play chess?)

Working within the limits of written communication, I'm not completely sure whether you meant this as genuine appreciation, or whether you were being sarcastic.

Assuming it was the former: Why, thanks! ❤️ (I used to play some chess as a child, but gave up on it quite soon. Nowadays, I really enjoy escape rooms and also logic puzzles. What, you wanted to get some real work done today instead? 😉)

[JakeQZ]

Thinking about this a bit more, I'm not convinced we can deprecate/drop support for the array arguments for list separator spacing.

Consider

color: rgba(0, 85, 0, 0.5); /* "legacy" syntax */
color: rgba(0 85 0 / 0.5); /* "modern syntax */

These examples use the typical human-readable format which is also used in MDN and W3C examples.

Both / and , are list separators. But if we stop allowing a distinction between them for output spacing, the above output would not be possible. The choice would be between

color: rgba(0 , 85 , 0 , 0.5); /* "legacy" syntax */
color: rgba(0 85 0 / 0.5); /* "modern syntax */

and

color: rgba(0, 85, 0, 0.5); /* "legacy" syntax */
color: rgba(0 85 0/ 0.5); /* "modern syntax */

both of which are ugly and not as humanly-readable.

I was mistaken about that, but wonder if it's possible to use @deprecated in the DocBlock for a partial deprecation...

As far as I know, that's not possible. @deprecated annotations always refer to one "thing" in its entirety, be it class, method, property or parameter.

Indeed that seems so. Moreover,

Structural Elements tagged with the @deprecated tag will be listed in the Deprecated elements report and their name will be shown as strike through,

So bending the rules won't work. And the tag does not have an inline option (e.g. {@deprecated} within a parameter description).

Wow. I couldn't think of a way of achieving the seemingly impossible...

Working within the limits of written communication, I'm not completely sure whether you meant this as genuine appreciation, or whether you were being sarcastic.

The former. See #866 (comment). I didn't think of simply deprecating the test method referred to in the documentation along with adding a deprecation note to the property.

(I used to play some chess as a child, but gave up on it quite soon. Nowadays, I really enjoy escape rooms and also logic puzzles. What, you wanted to get some real work done today instead? 😉)

(I wondered, because chess often involves seeing a cunning yet simple way through - and we could do with more participants in our online chess tourneys. Thanks for the puzzles link. Some are the same type that appear in daily newspapers along with the crossword. I occasionally try them. My Mum loves them, so I'll pass on the link.)

[JakeQZ]

Thinking about this a bit more, I'm not convinced we can deprecate/drop support for the array arguments for list separator spacing.

Maybe we can for 'after' (though not 'before'), but I'm not 100% convinced we may not have missed a case where the variation for different separators is actually needed to render human-readable CSS.

[sabberworm]

I didn’t follow any of the discussions leading up to this so I’m curious: what’s the rationale for this change?

[oliverklee]

what’s the rationale for this change?

I'm generally working on these things:
a) covering the existing code with tests,
b) replacing magic methods with explicit existing methods, and
c) making type declarations as correct and specific as possible (including native types where possible) so we can make use the interpreter's type checks and PHPStan's checks.

In the process, I added tests for the (hitherto) magic getters and setters of OutputFormat in #847, #862 and #867. Then I replaced the magic getters and setters with explicit methods with native type declarations in #871.

This lead me to discover that at least the test deprecated here relied on OutputFormat.sSpaceAfterListArgumentSeparator allowing arrays in addition to strings (which meant that the current type annotation string was either incorrect/incomplete, or the code using it was incorrect). So currently this test is blocking the explicit accessor methods with native type declarations.

I prefer to have types that are as narrow is possible, and to use native type declarations as much as possible.

So that's the rationale. :-)

@sabberworm Maybe you can shed some light on this: Does it make sense to enforce this property being string only in 9.0? Or is allowing arrays as well important, and we need to allow that in the getter and setter as well?

[sabberworm]

I suppose it’s sensible to allow customizing the whitespace for specific separators but I agree it should not clash with type annotations. IMO we could change setSpaceAfterListArgumentSeparator to only allow strings but allow overrides being registered with a separate method, e.g. setSpaceAfterListArgumentSeparatorOfType($type, $space).

[oliverklee]

Thanks! I propose we add a new typesafe property and method when we need it.

[JakeQZ]

Thanks! I propose we add a new typesafe property and method when we need it.

We'd need to add this before deprecating the possibility of the property being an array, wouldn't we? So the deprecation message can become

array is deprecated in version V8.8.0, will be changed to string only in V9.0.0. To set the spacing for specific separators, use sSpaceAfterListArgumentSeparatorOfType instead.

[oliverklee]

I propose we drop the functionality of setting this to an array without replacement altogether for the time being, and re-add it (with the corresponding method) when and if we need this.

[JakeQZ]

I propose we drop the functionality of setting this to an array without replacement altogether for the time being, and re-add it (with the corresponding method) when and if we need this.

For the reasons I mentioned in #876 (review), I think we need this, at least for the counterpart sSpaceBeforeListArgumentSeparator. And if we have it for one, we may as well for the other, as it's hardly any extra effort (just a case of 'rinse and repeat').

I think the ability to unminify CSS and/or render it in a human-readable format is an important feature of the library. Removing the option of different-spacing-according-to-separator breaks that feature.

Also, that feature is almost zero-cost in terms of maintenance. (By far the biggest maintenance cost is changes to the CSS spec for parsing, dictated by the GAFAMs, who are beyond reproach.)

I'd be more than happy to create a PR to add the ...SeparatorOfType alternative suggested by @sabberworm, whilst retaining the original array option (to be deprecated), and still using the magic methods (so it can easily be backported). Might take a couple of days. Once that's done, this PR can proceed. @oliverklee, have I persuaded you?

[oliverklee]

Yes, let's do this. 🙏 Please go ahead!

[oliverklee]

I‘d prefer the new method to be specific to passing an array, though (both in naming as well as the parameter type), instead of adding a general method that allows passing the type of the other parameter.

[sabberworm]

I‘d prefer the new method to be specific to passing an array, though (both in naming as well as the parameter type), instead of adding a general method that allows passing the type of the other parameter.

Sorry for the confusion. The method I suggested would take two strings. The $type name of the parameter was referring to the type of the list separator.

[JakeQZ]

I‘d prefer the new method to be specific to passing an array, though (both in naming as well as the parameter type), instead of adding a general method that allows passing the type of the other parameter.

Sorry for the confusion. The method I suggested would take two strings. The $type name of the parameter was referring to the type of the list separator.

In #880, I added an additional separate method accepting an array (only), whose values would override the default string set by the original method. This was simpler to implement than having methods to set the values in the array individually, which would have also needed a means to unset elements.