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

[BUGFIX] Fix comment parsing to support multiple comments

Open ziegenberg opened this issue 1 year ago • 6 comments

Because of an eager consumption of whitespace, the rule parsing would swallow a trailing comment, meaning the comment for the next rule would be affected. This patch addresses this by only consuming real whitespace without comments after a rule.

Fixes #173

ziegenberg avatar Aug 24 '24 11:08 ziegenberg

It would be nice to have this in an 8.6.1 release.

ziegenberg avatar Aug 24 '24 11:08 ziegenberg

Battle plan as discussed in our call right now:

  1. 8.x We add a consumeWhiteSpaceWithComments
  2. 8.x consumeWhiteSpace redirects to consumeWhiteSpaceWithComments, redirect it to the new method and mark it as deprecated
  3. 8.x change the respective call to not call the consume* method and have the corresponding lines directly instead
  4. 9.0 remove/change the deprecated method

oliverklee avatar Aug 25 '24 16:08 oliverklee

Battle plan as discussed in our call right now:

1. 8.x We add a `consumeWhiteSpaceWithComments`
2. 8.x `consumeWhiteSpace` redirects to `consumeWhiteSpaceWithComments`, redirect it to the new method and mark it as deprecated
3. 8.x change the respective call to not call the `consume*` method and have the corresponding lines directly instead
4. 9.0 remove/change the deprecated method

I don't think we should ever reintroduce a method with the same name as one that previously existed, with different default funcionality. It will catch out anyone doing a major update through several major versions, which happens even (and probably moreso) in sizeable companies as they defer the work required to support the update, deeming it low priority when updating requires some other changes.

So I think we may as well come up with a new method name now; maybe consumeWhitespaceOnly. (Unfortunately, PHP method names are not case sensitive, so we can't use consumeWhitespace.)

JakeQZ avatar Aug 26 '24 01:08 JakeQZ

@ziegenberg Do we still need this PR now that #671/#672 have been merged?

oliverklee avatar Aug 26 '24 15:08 oliverklee

I propose we move the discussion of the next steps here: #679

oliverklee avatar Aug 26 '24 21:08 oliverklee

#173 has finally been fixed by #1169; however, the suggestions here seem good for consideration moving forwards.

JakeQZ avatar Mar 16 '25 22:03 JakeQZ