Skip to content

[FEATURE] Add consumeWhitespaceWithComments and redirect consumeWhitespace (backported)#670

Closed
ziegenberg wants to merge 2 commits intoMyIntervals:v8.xfrom
ziegenberg:add-comsume-whitespacewithcomment
Closed

[FEATURE] Add consumeWhitespaceWithComments and redirect consumeWhitespace (backported)#670
ziegenberg wants to merge 2 commits intoMyIntervals:v8.xfrom
ziegenberg:add-comsume-whitespacewithcomment

Conversation

@ziegenberg
Copy link
Copy Markdown
Contributor

@ziegenberg ziegenberg commented Aug 25, 2024 •
edited
Loading

Having consumeWhitespace also consumes comments, and returning them was unexpected. This also "deprecates" the consumeWhitespace method and announces a breaking change for 9.0.0.

ziegenberg added a commit to ziegenberg/PHP-CSS-Parser that referenced this pull request Aug 25, 2024
@ziegenberg
[FEATURE] Add comsumeWhitespaceWithComments and redirect comsumeWhite…
62dddaf 
…space (MyIntervals#670)

Having comsumeWhitespace also consume comments and returning them was unexpected.

Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
ziegenberg added a commit to ziegenberg/PHP-CSS-Parser that referenced this pull request Aug 25, 2024
@ziegenberg
[TASK] Deprecate ParserState::comsumeWhitespace (MyIntervals#670)
b92f7c3 
Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
@ziegenberg ziegenberg force-pushed the add-comsume-whitespacewithcomment branch from a07190b to b92f7c3 Compare August 25, 2024 17:10
ziegenberg added a commit to ziegenberg/PHP-CSS-Parser that referenced this pull request Aug 25, 2024
@ziegenberg
[FEATURE] Add comsumeWhitespaceWithComments and redirect comsumeWhite…
119b1d2 
…space (MyIntervals#670)

Having comsumeWhitespace also consume comments and returning them was unexpected.

Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
ziegenberg added a commit to ziegenberg/PHP-CSS-Parser that referenced this pull request Aug 25, 2024
@ziegenberg
[TASK] Deprecate ParserState::comsumeWhitespace (MyIntervals#670)
79b271f 
Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
@ziegenberg ziegenberg force-pushed the add-comsume-whitespacewithcomment branch from b92f7c3 to 79b271f Compare August 25, 2024 17:11
@ziegenberg ziegenberg changed the title [FEATURE] Add comsumeWhitespaceWithComments and redirect comsumeWhitespace [FEATURE] Add consumeWhitespaceWithComments and redirect consumeWhitespace Aug 25, 2024
@ziegenberg
[FEATURE] Add comsumeWhitespaceWithComments and redirect comsumeWhite…
0fa736a 
…space (MyIntervals#670)

Having comsumeWhitespace also consume comments and returning them was unexpected.

Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
@ziegenberg
[TASK] Deprecate ParserState::comsumeWhitespace (MyIntervals#670)
d53d9ba 
Signed-off-by: Daniel Ziegenberg <daniel@ziegenberg.at>
@ziegenberg ziegenberg force-pushed the add-comsume-whitespacewithcomment branch from 79b271f to d53d9ba Compare August 25, 2024 17:19
@ziegenberg ziegenberg changed the title [FEATURE] Add consumeWhitespaceWithComments and redirect consumeWhitespace [FEATURE] Add consumeWhitespaceWithComments and redirect consumeWhitespace (backported) Aug 25, 2024
oliverklee
oliverklee requested changes Aug 25, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Could you also add a test for the new method? (Copying and modifying the test for the other method should be fine.)

Comment thread src/Parsing/ParserState.php
@ziegenberg
Copy link
Copy Markdown
Contributor Author

ziegenberg commented Aug 25, 2024

Thanks! Could you also add a test for the new method? (Copying and modifying the test for the other method should be fine.)

@ziegenberg ziegenberg closed this Aug 25, 2024
@ziegenberg ziegenberg deleted the add-comsume-whitespacewithcomment branch August 25, 2024 21:19
@ziegenberg ziegenberg restored the add-comsume-whitespacewithcomment branch August 25, 2024 21:19
@ziegenberg ziegenberg reopened this Aug 25, 2024
@ziegenberg
Copy link
Copy Markdown
Contributor Author

ziegenberg commented Aug 25, 2024

Sorry, clicked the wrong buttons...

@oliverklee
Copy link
Copy Markdown
Collaborator

oliverklee commented Aug 25, 2024

This should go into main first. Would you be willing to create a new PR targeting main? (If I switch the target of this PR, I fear that this would create a bunch of merge conflicts.)

@oliverklee
Copy link
Copy Markdown
Collaborator

oliverklee commented Aug 26, 2024

And (as far as I see) as this isn't necessary for the bugfix (please correct me if I'm wrong) and does not provide any user-/developer facing new feature, I'd prefer this to go into main only. Would you be okay with this, @ziegenberg?

@JakeQZ
Copy link
Copy Markdown
Collaborator

JakeQZ commented Feb 10, 2026

consumeWhiteSpace() was changed in #1477 to return the whitespace consumed rather than the comments, with an optional $comments parameter to be filled in with the comments, akin to the other ParserState methods.

ParserState has been marked @internal since v8.7 and is no longer part of the public API.

If there becomes a need somewhere to consume whitespace but not comments, a method can be implemented, but I'm not currently aware of any need.

@JakeQZ JakeQZ closed this Feb 10, 2026
@JakeQZ JakeQZ added the wontimplement Suggestions for enhancements that maintainers have agreed by consensus are not worthwhile for now label Feb 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oliverklee oliverklee oliverklee requested changes

Assignees

@ziegenberg ziegenberg

Labels

enhancement wontimplement Suggestions for enhancements that maintainers have agreed by consensus are not worthwhile for now

Projects

None yet

Milestone

9.2.0: New features, bug fixes and de...

Development

Successfully merging this pull request may close these issues.

3 participants

@ziegenberg @oliverklee @JakeQZ