NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[DependencyInjection] fixed PhpDumper + as_files + new lines in string arguments/properties/etc of Definitions#24517

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

Strate wants to merge4 commits intosymfony:3.4fromStrate:issue_24470_as_files_dumper_fix

Closed

[DependencyInjection] fixed PhpDumper + as_files + new lines in string arguments/properties/etc of Definitions#24517

Strate wants to merge4 commits intosymfony:3.4fromStrate:issue_24470_as_files_dumper_fix

Conversation

Copy link

Contributor

Strate commentedOct 11, 2017

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#24470
License	MIT
Doc PR	no

Handle the case, when exporting string contains new line, which cause
incorrect php code together withas_files option

fixes#24470

[DependencyInjection] fixed PhpDumper

31dd6ae

Handle the case, when exporting string contains new line, which causeincorrect php code together with `as_files` optionfixessymfony#24470

carsonbot added Status: Needs Review DependencyInjection Bug labels

Oct 11, 2017

fixed coding style

f427f09

nicolas-grekas added this to the3.4 milestone

Oct 12, 2017

Copy link

Member

nicolas-grekas commentedOct 12, 2017•
edited
Loading

If I remove your change in PhpDumper, I get this change in the fixtures file:

-return $this->services['foo'] = new \Foo('string with' . "\n" . 'new line');+return $this->services['foo'] = new \Foo('string with+');

The code without your change is thus perfectly fine.
This means the bug description (generating invalid PHP code?) is not proved yet.
We'd need a test case that actually generates invalid code, then a fix for it.
Possible? Otherwise, this means this fixes nothing in fact...

Copy link

Member

nicolas-grekas commentedOct 12, 2017•
edited
Loading

OH got it :) "new line" is missing when the patch is not applied!
This bug exists in 2.7 I think also, and not specific to as_files (see "explode" in "wrapServiceConditionals").

nicolas-grekas modified the milestones:3.4,2.7

Oct 12, 2017

Copy link

ContributorAuthor

Strate commentedOct 12, 2017•
edited
Loading

@nicolas-grekas yes. Generated code become invalid if test case changed from"string with\nnew line"
to"string with\nnl" - if length of after-\n part is less than 8, then generated code is invalid. I can add test case for that too.

UPD, just tested that, it compiles to:

return $this->services['foo2'] = new \Foo('string with

Added a test case, when broken PhpDumper generates invalid php code

1a0a591

Copy link

Member

nicolas-grekas commentedOct 12, 2017

Can you submit the fix on 2.7?

Strate changed the base branch from3.4 to2.7

October 12, 2017 11:10

Strate changed the base branch from2.7 to3.4

October 12, 2017 11:10

Strate mentioned this pull request

Oct 12, 2017

[DI] Fix possible incorrect php-code when dumped strings contains newlines#24532

Closed

Copy link

ContributorAuthor

Strate commentedOct 12, 2017•
edited
Loading

@nicolas-grekas I've made separate PR#24532
Unfortunatelly no tests, becasuse I don't know which case should be tested.

[DI] PhpDumper. Make generated code compatible with coding standards

e6e649b

nicolas-grekas added a commit that referenced this pull request

Oct 12, 2017

bug#24532[DI] Fix possible incorrect php-code when dumped strings c…

262b4f3

…ontains newlines (Strate)This PR was squashed before being merged into the 2.7 branch (closes#24532).Discussion----------[DI] Fix possible incorrect php-code when dumped strings contains newlines| Q             | A| ------------- | ---| Branch?       | 2.7| Bug fix?      | yes| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks?    | no| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| Tests pass?   | yes| Fixed tickets | ?| License       | MIT| Doc PR        | noSee discussion#24517<!--- Bug fixes must be submitted against the lowest branch where they apply  (lowest branches are regularly merged to upper ones so they get the fixes too).- Features and deprecations must be submitted against the 3.4,  legacy code removals go to the master branch.- Please fill in this template according to the PR you're about to submit.- Replace this comment by a description of what your PR is solving.-->Commits-------345f2fc [DI] Fix possible incorrect php-code when dumped strings contains newlines

fabpot closed this

Oct 12, 2017

fabpot reopened this

Oct 12, 2017

Copy link

Member

xabbuh commentedOct 12, 2017

Isn't this fixed by#24532?

Copy link

ContributorAuthor

Strate commentedOct 12, 2017

@xabbuh this was created firstly, and this targets to 3.4

Copy link

Member

xabbuh commentedOct 12, 2017

Lower branches are merged up regularly. So the other fix will land in 3.4 eventually.

Copy link

ContributorAuthor

Strate commentedOct 12, 2017

But here is more unit test, target to as_files dumper option

Copy link

Member

chalasr commentedOct 17, 2017•
edited
Loading

The test added on 2.7 seems good enough, the value ofas_files does not matter regarding the fixed bug in fact. This has been merged up to master by Nicolas already.
Closing this one.