-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix #79934: CRLF-only line in heredoc causes parsing error #5944
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
Conversation
Bug reference: https://bugs.php.net/bug.php?id=79934 I'm not sure how to create a test case for this bug. This is my first time contributing to php-src - let me know if I need to change anything. |
Also, while investigating this bug I came across |
I think that might actually be the only bug here. Does fixing that function also resolve this problem? For the test, you'll probably want to use |
22acc70
to
b185b4c
Compare
That does seem to fix it. I changed the commit to fix |
Zend/tests/bug79934.phpt
Outdated
--FILE-- | ||
<?php | ||
eval("\$s = <<<HEREDOC\r\n a\r\n\r\n b\r\n HEREDOC;"); | ||
var_dump($s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to add variations with \n and \r as well and use something like addcslashes(
for the output, so we see the linebreaks more explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the test to include cases with "\n", "\r" and both "\r\n" as well as a whitespace-only line.
Fixes the function `next_newline()` in zend_language_scanner.l. The function now correctly returns a newline_len of 2 for "\r\n".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
In
strip_multiline_string_indentation()
(Zend/zend_language_scanner.l)the case that is supposed to ignore whitespace-only lines now correctly
handles CRLF-only lines.