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

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

Closed
wants to merge 4 commits into
base: 3.4
from

Conversation

Projects
None yet
6 participants
@Strate
Contributor

Strate commented Oct 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 with as_files option

fixes #24470

[DependencyInjection] fixed PhpDumper
Handle the case, when exporting string contains new line, which cause
incorrect php code together with `as_files` option

fixes #24470

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Oct 12, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 12, 2017

Member

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...

Member

nicolas-grekas commented Oct 12, 2017

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...

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 12, 2017

Member

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").

Member

nicolas-grekas commented Oct 12, 2017

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 nicolas-grekas modified the milestones: 3.4, 2.7 Oct 12, 2017

@Strate

This comment has been minimized.

Show comment
Hide comment
@Strate

Strate Oct 12, 2017

Contributor

@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
Contributor

Strate commented Oct 12, 2017

@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
@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 12, 2017

Member

Can you submit the fix on 2.7?

Member

nicolas-grekas commented Oct 12, 2017

Can you submit the fix on 2.7?

@Strate Strate changed the base branch from 3.4 to 2.7 Oct 12, 2017

@Strate Strate changed the base branch from 2.7 to 3.4 Oct 12, 2017

@Strate

This comment has been minimized.

Show comment
Hide comment
@Strate

Strate Oct 12, 2017

Contributor

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

Contributor

Strate commented Oct 12, 2017

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

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…
…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        | no

See 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 fabpot closed this Oct 12, 2017

@fabpot fabpot reopened this Oct 12, 2017

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Oct 12, 2017

Member

Isn't this fixed by #24532?

Member

xabbuh commented Oct 12, 2017

Isn't this fixed by #24532?

@Strate

This comment has been minimized.

Show comment
Hide comment
@Strate

Strate Oct 12, 2017

Contributor

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

Contributor

Strate commented Oct 12, 2017

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

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Oct 12, 2017

Member

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

Member

xabbuh commented Oct 12, 2017

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

@Strate

This comment has been minimized.

Show comment
Hide comment
@Strate

Strate Oct 12, 2017

Contributor

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

Contributor

Strate commented Oct 12, 2017

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

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Oct 17, 2017

Member

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

Member

chalasr commented Oct 17, 2017

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

@chalasr chalasr closed this Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment