Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TASK] Do not allow string values for rules anymore #353

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

oliverklee
Copy link
Contributor

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

@see MyIntervals/emogrifier#1144

This reverts commit 67a6e95.

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

@see MyIntervals/emogrifier#1144

This reverts commit 67a6e95.
@sabberworm
Copy link
Contributor

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
Copy link
Contributor

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

@oliverklee oliverklee deleted the branch MyIntervals:main February 7, 2024 11:36
@oliverklee oliverklee closed this Feb 7, 2024
@oliverklee oliverklee reopened this Feb 7, 2024
@oliverklee oliverklee changed the base branch from master to main February 7, 2024 22:35
@JakeQZ
Copy link
Contributor

JakeQZ commented Feb 29, 2024

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.

@oliverklee oliverklee marked this pull request as draft February 29, 2024 11:30
@oliverklee
Copy link
Contributor Author

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 oliverklee self-assigned this Aug 25, 2024
@oliverklee oliverklee changed the title Do not allow string values for rules anymore [TASK] Do not allow string values for rules anymore Aug 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants