Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

pevdh
Copy link
Contributor

@pevdh pevdh commented Aug 5, 2020

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.

@pevdh
Copy link
Contributor Author

pevdh commented Aug 5, 2020

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.

@pevdh
Copy link
Contributor Author

pevdh commented Aug 5, 2020

Also, while investigating this bug I came across next_newline in zend_language_scanner.l. This function is supposed to return newline_len = 2 for "\r\n", but always returns 1. I'm not sure if this unrelated bug has any unintended consequences.

@nikic
Copy link
Member

nikic commented Aug 5, 2020

Also, while investigating this bug I came across next_newline in zend_language_scanner.l. This function is supposed to return newline_len = 2 for "\r\n", but always returns 1. I'm not sure if this unrelated bug has any unintended consequences.

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 eval() with explicit \r\n, to make sure the file doesn't get reformatted.

@pevdh pevdh force-pushed the fix-bug-79934 branch 2 times, most recently from 22acc70 to b185b4c Compare August 5, 2020 20:53
@pevdh
Copy link
Contributor Author

pevdh commented Aug 5, 2020

That does seem to fix it. I changed the commit to fix next_newline instead & added a test.

--FILE--
<?php
eval("\$s = <<<HEREDOC\r\n a\r\n\r\n b\r\n HEREDOC;");
var_dump($s);
Copy link
Member

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.

Copy link
Contributor Author

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".
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@php-pulls php-pulls closed this in 06ade15 Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants