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.

Sign up for GitHub

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

Jump to bottom

HeredocToNowdocFixer - Handle space in heredoc token #1879

Merged
merged 1 commit into from May 23, 2016
Merged

HeredocToNowdocFixer - Handle space in heredoc token #1879

merged 1 commit into from May 23, 2016

Conversation

SpacePossum
Copy link
Contributor

Closes #1869

ping @GrahamCampbell and @gharlan

@GrahamCampbell
Copy link
Contributor

Some of these test cases are invalid php.

@GrahamCampbell
Copy link
Contributor 

There was 1 failure:
1) Symfony\CS\Tests\Fixer\Symfony\HeredocToNowdocFixerTest::testFix with data set #8 ('<?php\n$html = <<<'HTML'\na\nHTM...HTML;\n')
Failed asserting that '
Parse error: syntax error, unexpected ',' in /home/travis/build/FriendsOfPHP/PHP-CS-Fixer/cs_fixer_tmp_Khtpgj on line 6
Errors parsing /home/travis/build/FriendsOfPHP/PHP-CS-Fixer/cs_fixer_tmp_Khtpgj
' is null.
/home/travis/build/FriendsOfPHP/PHP-CS-Fixer/Symfony/CS/Tests/Fixer/AbstractFixerTestBase.php:95
/home/travis/build/FriendsOfPHP/PHP-CS-Fixer/Symfony/CS/Tests/Fixer/Symfony/HeredocToNowdocFixerTest.php:29

@gharlan
Copy link
Contributor

gharlan commented Apr 20, 2016

I think the fixer should not remove the spaces.

@GrahamCampbell
Copy link
Contributor

I think the fixer should not remove the spaces.

Not removing the spaces results in a syntax error.

@gharlan
Copy link
Contributor

gharlan commented Apr 20, 2016

Oh, ok, didn't know that.

gharlan
gharlan reviewed Apr 20, 2016
View reviewed changes
Symfony/CS/Tests/Fixer/Symfony/HeredocToNowdocFixerTest.php

EOF
, <<<'EOF'
<?php $a = <<< "TEST"
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, this is invalid. why isn't it failing in the travis build with enabled linting?

Copy link
Contributor

Choose a reason for hiding this comment

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

errr, not sure

Copy link
Contributor

Choose a reason for hiding this comment

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

see #1869

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<?php $a = <<<           "TEST"
Foo
TEST;

echo $a;die;

runs for me, what part is invalid?

Copy link
Contributor

Choose a reason for hiding this comment

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

What about with single quotes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same, all ok (PHP 5.5.9-1ubuntu4.14 (cli) (built: Oct 28 2015 01:34:46)

Copy link
Contributor

Choose a reason for hiding this comment

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

@SpacePossum is right, both are valid.

@gharlan
Copy link
Contributor

gharlan commented Apr 20, 2016

Not removing the spaces results in a syntax error.

it is valid: https://3v4l.org/GlPME

So, I still think we should not remove the spaces.

@GrahamCampbell
Copy link
Contributor

So, I still think we should not remove the spaces.

How come my other test file is getting broken then?

@gharlan
Copy link
Contributor

gharlan commented Apr 20, 2016

Because the fixer does not handle the spaces correctly.

@SpacePossum
Copy link
Contributor Author

How come my other test file is getting broken then?

because the opening quote was misplaced;
current wrong:

<<< "    HTML"

PR fixes to:

<<< "HTML"

suggested is:

<<<     "HTML"

input

<<<     HTML

I guess?

@gharlan
Copy link
Contributor

gharlan commented Apr 20, 2016

suggested is:

<<<     'HTML'

(single quotes)

@SpacePossum
Copy link
Contributor Author

course :)
updated, I have the feeling the replace can be done more efficient, if so maybe someone can suggest how?

@gharlan
Copy link
Contributor

gharlan commented Apr 20, 2016

I have the feeling the replace can be done more efficient

I don't think so.

👍 🚢

@GrahamCampbell
Copy link
Contributor

👍

@GrahamCampbell
Copy link
Contributor

Is this good to merge @keradus?

@keradus
Copy link
Member

keradus commented May 22, 2016

Yes, please rebase.
I got falsy impression thanks to our stalker ;)

@keradus keradus added this to the v1.12 milestone May 22, 2016
@GrahamCampbell
Copy link
Contributor

I got falsy impression thanks to our stalker ;)

I wonder if I can get gitHub to actually delete all the mess they've made.

@keradus
Copy link
Member

keradus commented May 22, 2016

One one or another please rebase and put your thumb for #1903 ;)

@keradus keradus changed the title [Bug] Handle space in heredoc token. HeredocToNowdocFixer - Handle space in heredoc token May 22, 2016
@SpacePossum
Copy link
Contributor Author

rtm :)
dunno why the test don't run atm btw

@keradus
Copy link
Member

keradus commented May 23, 2016

why based on #1896 ?

@SpacePossum
Copy link
Contributor Author

SpacePossum commented May 23, 2016 •
edited

I guess I messed up the rebase, wasn't a good weekend for development, lets see

fixed

@GrahamCampbell
Copy link
Contributor

👍

@keradus keradus added RTM Ready To Merge and removed RTM Ready To Merge labels May 23, 2016
@keradus
Copy link
Member

keradus commented May 23, 2016

Thank you @SpacePossum.

@keradus keradus merged commit a550caf into PHP-CS-Fixer:1.12 May 23, 2016
keradus added a commit that referenced this pull request May 23, 2016
@keradus
bug #1879 HeredocToNowdocFixer - Handle space in heredoc token (Space…
5448a0f 
…Possum)

This PR was merged into the 1.12 branch.

Discussion
----------

HeredocToNowdocFixer - Handle space in heredoc token

Closes #1869

ping @GrahamCampbell and @gharlan

Commits
-------

a550caf Handle with space in HereDoc name.
@SpacePossum SpacePossum deleted the 1_12_bug_1869_Heredoc_spaces branch May 24, 2016 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
kind/bug
Projects
None yet
Milestone
v1.12.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@SpacePossum @GrahamCampbell @gharlan @keradus