8000 [DependencyInjection] fix dumped YAML string by xabbuh · Pull Request #17823 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DependencyInjection] fix dumped YAML string #17823

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
Merged
merged 1 commit into from
Feb 17, 2016
Merged

[DependencyInjection] fix dumped YAML string #17823

merged 1 commit into from
Feb 17, 2016

Conversation

xabbuh
Copy link
Member
@xabbuh xabbuh commented Feb 17, 2016
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

@stof
Copy link
Member
stof commented Feb 17, 2016

👍

@Tobion
Copy link
Contributor
Tobion commented Feb 17, 2016

Some other calls don't use dump as well like $definition->getFactoryMethod(). Shouldn't they be wrapped as well?

@xabbuh
Copy link
Member Author
xabbuh commented Feb 17, 2016

If I am not mistaken parameters aren't supported for the other options. Though wrapping all these places doesn't hurt at all. I'll update.

@Tobion
Copy link
Contributor
Tobion commented Feb 17, 2016

I thought it's about properly wrapping strings in quotation marks so they don't start with invalid @ etc.

@xabbuh
Copy link
Member Author
xabbuh commented Feb 17, 2016

Exactly, though I think the places were I added this then too usually wouldn't contain forbidden characters anyway. But it shouldn't hurt to have this handling here.

@Tobion
Copy link
Contributor
Tobion commented Feb 17, 2016

Thank you @xabbuh.

@Tobion Tobion merged commit b1bb135 into symfony:2.3 Feb 17, 2016
Tobion added a commit that referenced this pull request Feb 17, 2016
@Tobion
bug #17823 [DependencyInjection] fix dumped YAML string (xabbuh)
d3114d1 
This PR was merged into the 2.3 branch.

Discussion
----------

[DependencyInjection] fix dumped YAML string

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Commits
-------

b1bb135 [DependencyInjection] fix dumped YAML string
@xabbuh xabbuh deleted the di-yaml-syntax branch February 17, 2016 21:18
This was referenced Feb 28, 2016
Merged
Merged
Merged
Merged
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
DependencyInjection Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xabbuh @stof @Tobion @carsonbot
0