Inconsistent handling of commented CSS rules

[8ctopus]

$input = <<<CSS
a {
    /* margin: 10px 10px 10px 0; */
    outline: none;
}

CSS;

$document = (new \Sabberworm\CSS\Parser($input))
    ->parse();

$format = OutputFormat::createPretty();

echo $document->render($format);

becomes:

a {
        /* margin: 10px 10px 10px 0; */
        outline: none;
}

while if you update the input to remove the outline property,

a {
    /* margin: 10px 10px 10px 0; */
}

it becomes:

a {}

so any commented rule(s) not followed by a CSS rule are removed from the resulting rule set.

Problem occurs here because getRules() returns an empty array

PHP-CSS-Parser/src/RuleSet/RuleSet.php

Line 308 in 0ae1fde

foreach ($this->getRules() as $rule) {

[JakeQZ]

The current behaviour is sort-of by design, but I agree should be improved. It assumes that a comment preceding a Rule is commenting that Rule (and is not a commented-out Rule), so it becomes associated with that Rule. Thus, when the Rule is removed, the comment that goes with it is also removed.

Looking at the code, I think a comment in the following would also be dropped at the parsing stage:

a {
    /* This has no properties yet. */
}

To resolve these issues, I think we would need to modify RuleSet so that the list of Rules can be a list of Rule|Comments. And give the removeRule() method and option to keep the comment (which will effectively replace the Rule with the Comment(s) it had, if any).

[oliverklee]

To resolve these issues, I think we would need to modify RuleSet so that the list of Rules can be a list of Rule|Comments.

I'd prefer we replace the union type with a shared interface. That would allow us to use native type declarations and make it clear in the interface which methods are relevant.

[JakeQZ]

I'd prefer we replace the union type with a shared interface.

I was thinking that. I just wrote the union as shorthand.

[8ctopus]

Honestly, I reported it but don't need it fixed, especially if it adds a lot of complexity to the code.

[JakeQZ]

Hmm...

We also need to support this kind of construct:

p {
    /* This is a comment for the declaration */
    font-size: 14px; /* smaller than usual */
    /* This is a comment for the next declaration */
    color: green;
}

... so that the comments are correctly applied (i.e. comment before, comment after). So a consumeWhitespaceUntilLineFeed() method will probably be useful to pick up the comment after the declaration that it belongs to.

[JakeQZ]

We also need to support this kind of construct:

#1505 will potentially allow for trailing comments on the same line to be correctly associated.

[JakeQZ]

To resolve these issues, I think we would need to modify RuleSet so that the list of Rules can be a list of Rule|Comments.

I'd prefer we replace the union type with a shared interface. That would allow us to use native type declarations and make it clear in the interface which methods are relevant.

On consideration, I don't think an interface for an object which is either a Rule or a Comment would be a good design, as it would make the types somewhat codependent: Rule uses Comment but Comment should not need to know about Rule or be contaminated to implement an interface intended for Rule.

Instead, I think these are the options:

• Allow a Rule to contain only comments;
• Create a class alongside Rule which is a DeclarationBlockComment, with both implementing a common interface (e.g. DeclarationBlockItem).

I much prefer the second.

Note that Rule is a misnomer; it should be something like PropertyDeclaration.

[oliverklee]

Create a class alongside Rule which is a DeclarationBlockComment

Sounds good!