Skip to content

Conversation

czosel
Copy link
Collaborator

@czosel czosel commented Jan 8, 2020

Fixes #468

@coveralls
Copy link

coveralls commented Jan 8, 2020

Coverage Status

Coverage decreased (-0.02%) to 96.142% when pulling 6e372ba on czosel:fix-heredoc-trailing-newline into c4d95d8 on glayzzle:master.

@ichiriac
Copy link
Member

ichiriac commented Jan 9, 2020

Hi @czosel, problem comes from here : https://github.com/glayzzle/php-parser/blob/master/src/parser/scalar.js#L227

In this function I remove the last line break, but here the last char is the indentation, so you should before checking add an extra check on indentation, remove the trailing indentation, and then keep the strip of last line break.

@ichiriac ichiriac self-requested a review January 9, 2020 16:55
@ichiriac ichiriac added this to the 3.0.0 - stable milestone Jan 9, 2020
@czosel czosel force-pushed the fix-heredoc-trailing-newline branch from 4dfb2c6 to 8101f4f Compare January 9, 2020 19:32
@czosel
Copy link
Collaborator Author

czosel commented Jan 9, 2020

Thanks for the pointer @ichiriac! I added a fix for nowdoc, but heredoc now I still have to figure out heredoc. Do you have another pointer? 😉

Update: Tests in prettier are passing now, because we're printing heredoc differently using raw instead of value - but we should fix heredoc here as well for consistency.

@ichiriac
Copy link
Member

ichiriac commented Jan 9, 2020

Not sure, but from reading the code it may be handled here : https://github.com/glayzzle/php-parser/blob/master/src/parser/scalar.js#L329

From case this.tok.T_START_HEREDOC: ->
if (this.lexer.curCondition === "ST_NOWDOC") {
} else {
--> return this.read_encapsed_string(this.tok.T_END_HEREDOC);
enter into a loop :
while (this.token !== expect && this.token !== this.EOF) {
      value.push(this.read_encapsed_string_item(true));
    }
and then using if (this.token === this.tok.T_ENCAPSED_AND_WHITESPACE) 

The problem here is that you may also have tokens T_ENCAPSED_AND_WHITESPACE at the start;, middle, and at the end ... so you should include the fix here : https://github.com/glayzzle/php-parser/blob/master/src/parser/scalar.js#L435

By testing if the last node kind is a string and something like this :

if (value.length > 0 && value[value.length -1].kind === 'string') {
 const node = value[value.length -1];
 if ( this.lexer.heredoc_label.indentation > 0) {
     node.value = node.value.substring(0, node.value.length - this.lexer.heredoc_label.indentation;
 }
 // because it was never handled before
 const lastCh = ... 
}

@czosel czosel force-pushed the fix-heredoc-trailing-newline branch from 8101f4f to b8560a8 Compare January 10, 2020 08:10
@czosel czosel force-pushed the fix-heredoc-trailing-newline branch from b8560a8 to 6e372ba Compare January 10, 2020 08:14
@czosel
Copy link
Collaborator Author

czosel commented Jan 10, 2020

Thanks again for the pointers @ichiriac - I made the changes. I didn't need to remove the indentation for heredoc, because that's already done in remeve_heredoc_leading_whitespace_chars.

Copy link
Member

@ichiriac ichiriac left a comment

Choose a reason for hiding this comment

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

perfect 👍 ready to merge 💯

@ichiriac ichiriac merged commit 0b93e79 into glayzzle:master Jan 10, 2020
@czosel czosel deleted the fix-heredoc-trailing-newline branch January 10, 2020 09:17
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.

Flexible nowdoc contains extra linebreak

3 participants