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

Fix #76965: INI_SCANNER_RAW doesn't strip trailing whitespace #3572

Closed
wants to merge 1 commit into from

Conversation

3 participants
@cmb69
Copy link
Contributor

commented Oct 3, 2018

Contrary to INI_SCANNER_NORMAL and INI_SCANNER_TYPED,
INI_SCANNER_RAW didn't strip whitespace at the end of the line, or
before a line comment, which is undesirable. It is a particular
problem if the value is enclosed in double-quotes, since in this case
the quotes didn't get stripped.

Fix #76965: INI_SCANNER_RAW doesn't strip trailing whitespace
Contrary to `INI_SCANNER_NORMAL` and `INI_SCANNER_TYPED`,
`INI_SCANNER_RAW` didn't strip whitespace at the end of the line, or
before a line comment, which is undesirable.  It is a particular
problem if the value is enclosed in double-quotes, since in this case
the quotes didn't get stripped.
@cmb69

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2018

Hmm, that breaks bug51094.phpt. Reading the reasoning for the fix of https://bugs.php.net/63512, it seems the current behavior is intended (at least for BC reasons). @adoy What do you think?

@nikic

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

@cmb69 I don't think the current behavior, where "foo" ; bar is not handled correctly, is intentional. The proposed implementation is just not entirely correct, it needs to account for the "foo ; bar" case.

@adoy

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

@cmb69 as mentionned by @nikic "foo" ; bar result is not intended and is a bug that should be fix. But the tested behaviour in bug51094.phpt is the one intended.

@adoy

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2018

Hi @cmb69 and @nikic here is a fix I came up with for this issue. What do you think ? I did not created a new PR to avoid double discussion.

@cmb69

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2018

@adoy This looks way better than my attempt. Thanks! I'm closing this PR, so you can submit your solution.

@cmb69 cmb69 closed this Oct 6, 2018

@cmb69 cmb69 deleted the cmb69:bug-76965 branch Oct 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.