Add interface type for $element parameter of getAllValues(), or deprecate it?

[JakeQZ]

When passed (recursively) through CSSBlockList::allValues(), this becomes a union type: CSSList|Rule|RuleSet|Value.

To help with static analysis, we could introduce an (empty) interface that these classes would all implement, e.g. CSSElement.

But I'm wondering if this method has much practical use. The Values returned are without any context information, so you would get objects representing things like 10px, without any idea of where they occur in the CSS. I suppose, since they are objects, you can modify them, with changes reflected in the rendered CSS, so you could use this method to replace, say, all instances of 10px with 12px. Is that a plausible use case? Are there other use cases for this?

It would not be much effort to introduce a CSSElement interface for the static analysis. I'm just wondering if this feature has any value, and could perhaps instead be deprecated and removed...

[oliverklee]

I'd like to replace allValues() with a (still-to-be-written) method getAllValues() that works similar to getAllRuleSets() etc.

Still, I think having the interface is helpful both for static analysis as well as the reader.

[JakeQZ]

I'd like to replace allValues() with a (still-to-be-written) method getAllValues() that works similar to getAllRuleSets() etc.

Document does have a getAllValues() method, but this calls on to the allValues() method of the parent CSSBlockList class to do the bulk of the work.

I'm just wondering if this method actually has much value (pardon the pun), before investing time in refactoring.

Still, I think having the interface is helpful both for static analysis as well as the reader.

Adding one shouldn't be much effort, and will help as a pre-patch for #1194. The types involved seem to be almost every CSS entity (?) for which the library has a class, with the exception of statement at-rules. I'm wondering what would be the best name for such an interface type...

[sabberworm]

But I'm wondering if this method has much practical use.

FWIW, that was the initial use-case I had when I created the parser. I had wanted to create a minimized preview of a website. None of the attempts at scaling an <iframe> worked out. So instead I just scaled all absolute lengths I found in the CSS. That’s why there is still the “Shrink all absolute sizes to half” example in the readme.

[JakeQZ]

FWIW, that was the initial use-case I had when I created the parser. I had wanted to create a minimized preview of a website. None of the attempts at scaling an <iframe> worked out. So instead I just scaled all absolute lengths I found in the CSS. That’s why there is still the “Shrink all absolute sizes to half” example in the readme.

Ah, I see. On that basis then, we should

• Create an interface for all types that are or may contain Values;
• Replace allValues() with a getAllValues() method that avoids passing the array by reference, as was done for getAllRuleSets() (and can also be done for allDeclarationBlocks() and allSelectors()).

Keeping this open for suggestions for the above interface name...

[JakeQZ]

Keeping this open for suggestions for the above interface name...

The parameter for Document::getAllValues() and CSSBlockList::allValues() is named $element. So perhaps CSSElement. It doesn't seem to belong in any namespace, but could inherit Renderable (since all elements are renderable), which also resides in the project root namespace.

[JakeQZ]

It would be good to deconflate the getAllValues() parameter which is either a string or a CSSElement, particularly as they could actually be used independently together.

Doing so would require a deprecation step. The easiest way would be to deprecate the $element parameter being a string, whilst adding a $searchString parameter for that functionality. Though it would result in the parameter order $element, $searchInFunctionArguments, $searchString, whereas $element, $searchString, $searchInFunctionArguments would be a more natural ordering, but also more of a breaking change.

[oliverklee]

I'd be in favor of deprecating and dropping the parameter, and then adding a findValuesByElement() method (if we find that this functionality is actually needed).

[JakeQZ]

For getAllValues(), we can't quite follow the refactoring process that was done for getAllDeclarationBlocks() (#990) and getAllRuleSets() (#1050), since getAllValues() has some (optional) parameters.

I propose retaining these, but changing the method signature to avoid their conflation, whilst also maintaining a logical parameter order.

In 8.8 we have

public function getAllValues(?CSSElement|?string $element = null, bool $searchInFunctionArguments = false): array

In 9.0 we would like (I think)

public function getAllValues(
    ?CSSElement $element = null, 
    ?string $ruleNameFilter = null, 
    bool $searchInFunctionArguments = false
): array

We can make the transition via a hybrid method in 8.9:

public function getAllValues(
    ?CSSElement|?string $elementOrRuleNameFilter = null,
    ?string|bool $ruleNameFilterOrSearchInFunctionArguments = null,
    bool $searchInFunctionArguments = false
): array

The implementation can inspect the argument types to see if the old or new signature is being used. The old signature would be marked as deprecated.

In 9.0, the messy logic can be removed.

PR on its way...

[JakeQZ]

#1241

[JakeQZ]

Closing as completed via #1231, #1240, #1241, #1243 and #1244.