PHP-CSS-Parser
PHP-CSS-Parser copied to clipboard
[TASK] Do not allow string values for rules anymore
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.
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) {
}
}
AFAICT the type for mValue
should be Value|string
. IMHO we should also get rid of null
and just use ""
for the default value.
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 Rule
s and failing to set their values. Rule
s 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.
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.