PHP-CSS-Parser icon indicating copy to clipboard operation
PHP-CSS-Parser copied to clipboard

[TASK] Do not allow string values for rules anymore

Open oliverklee opened this issue 3 years ago • 4 comments

String values had not been allowed for rules, and should not be. (Passing string values was a bug in the Emogrifier library.)

@see https://github.com/MyIntervals/emogrifier/pull/1144

This reverts commit 67a6e95f8ad2c9c3d8d745f1943eda8da81383a4.

oliverklee avatar Jan 07 '22 17:01 oliverklee

I’m not sure this is true. When the rule value is an identifier, it will be a string stored in mValue:

.test {
white-space: nowrap;
}

will parse into the following Rule:

          object(Sabberworm\CSS\Rule\Rule)#8 (7) {
            ["sRule":"Sabberworm\CSS\Rule\Rule":private]=>
            string(11) "white-space"
            ["mValue":"Sabberworm\CSS\Rule\Rule":private]=>
            string(6) "nowrap"
            ["bIsImportant":"Sabberworm\CSS\Rule\Rule":private]=>
            bool(false)
            ["aIeHack":"Sabberworm\CSS\Rule\Rule":private]=>
            array(0) {
            }
            ["iLineNo":protected]=>
            int(2)
            ["iColNo":protected]=>
            int(19)
            ["aComments":protected]=>
            array(0) {
            }
          }

sabberworm avatar Jan 10 '22 10:01 sabberworm

AFAICT the type for mValue should be Value|string. IMHO we should also get rid of null and just use "" for the default value.

sabberworm avatar Jan 10 '22 10:01 sabberworm

When the rule value is an identifier, it will be a string stored in mValue:

Would introducing an Identifier (or Keyword) subclass of Value help resolve this? Such a class would just wrap a string, but also allow the unique Value superclass type to be used whenever a value is expected or provided (e.g. see #507 which results in lots of type specifications of Value|string).

IMHO we should also get rid of null and just use "" for the default value.

It seems that the default, with no value having been set, does not represent a valid property declaration. This would not arise from parsing, since Value::parseValue would throw an exception, but may arise from manipulation by user code instantiating additional Rules and failing to set their values. Rules with a missing value would currently be rendered as key:;, and discarded upon reparsing - but should probably be rendered as an empty string.

The constructor enforces that a key is provided. Maybe it should also enforce that a value is provided - though that would be a BC.

JakeQZ avatar Feb 29 '24 00:02 JakeQZ

Marking this as draft for now. I'd like to pick this up again when we've covered the code with more unit tests. After that, I'd like to reduce multi-types as much as possible in order to simplify things and help static analysis.

oliverklee avatar Feb 29 '24 11:02 oliverklee