Skip to content

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

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
Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

Conversation

Strate
Copy link
Contributor

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

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

fixes symfony#24470
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Oct 12, 2017
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 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...

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 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 nicolas-grekas modified the milestones: 3.4, 2.7 Oct 12, 2017
@Strate
Copy link
Contributor Author

Strate commented Oct 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

@nicolas-grekas
Copy link
Member

Can you submit the fix on 2.7?

@Strate Strate changed the base branch from 3.4 to 2.7 October 12, 2017 11:10
@Strate Strate changed the base branch from 2.7 to 3.4 October 12, 2017 11:10
@Strate Strate mentioned this pull request Oct 12, 2017
Closed
@Strate
Copy link
Contributor Author

Strate commented Oct 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.

nicolas-grekas added a commit that referenced this pull request Oct 12, 2017
@nicolas-grekas
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        | 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
Copy link
Member

xabbuh commented Oct 12, 2017

Isn't this fixed by #24532?

@Strate
Copy link
Contributor Author

Strate commented Oct 12, 2017

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

@xabbuh
Copy link
Member

xabbuh commented Oct 12, 2017

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

@Strate
Copy link
Contributor Author

Strate commented Oct 12, 2017

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

@chalasr
Copy link
Member

chalasr commented Oct 17, 2017 •
edited
Loading

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
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
6 participants
@Strate @nicolas-grekas @xabbuh @chalasr @fabpot @carsonbot