8000 [Console] Add ArgvInput::__toString, fixes #7257 by Seldaek · Pull Request #7648 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Console] Add ArgvInput::__toString, fixes #7257 #7648

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
Apr 12, 2013
Merged

[Console] Add ArgvInput::__toString, fixes #7257 #7648

merged 1 commit into from
Apr 12, 2013

Conversation

Seldaek
Copy link
Member
@Seldaek Seldaek commented Apr 12, 2013

No description provided.

fabpot
fabpot reviewed Apr 12, 2013
View reviewed changes
src/Symfony/Component/Console/Input/ArgvInput.php
8000
*
* @return string
*/
public function __toString()
Copy link
Member

Choose a reason for hiding this comment

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

what about having a __toString() on all other Input classes.

Copy link
Member Author

Choose a reason for hiding this comment

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

String inherits already, I'll see what I can do about ArrayInput. IMO it could inherit from Argv but maybe I missed something

@Seldaek
Copy link
Member Author
Seldaek commented Apr 12, 2013

Updated

fabpot added a commit that referenced this pull request Apr 12, 2013
@fabpot
merged branch Seldaek/argv-tostring (PR #7648)
6cf491d 
This PR was merged into the master branch.

Discussion
----------

[Console] Add ArgvInput::__toString, fixes #7257

Commits
-------

659eb66 [Console] Add ArgvInput::__toString and ArrayInput::__toString, fixes #7257
@fabpot fabpot closed this Apr 12, 2013
@fabpot fabpot merged commit 659eb66 into symfony:master Apr 12, 2013
@igorw
Copy link
Contributor
igorw commented Apr 12, 2013

Why not escapeshellarg?

I believe this patch incorrectly escapes. You cannot escape quotes inside strings on the shell. so to escape I don't even, you would have to do either "I don't even" or 'I don'\''t even'. escapeshellarg does the latter.

@Seldaek
Copy link
Member Author
Seldaek commented Apr 12, 2013

I wanted something more targetted because if you escapeshellarg --foo you end up with '--foo' and then it's an argument and not an option. But yeah it's probably best to just use it on argument and then nothing on options. I'll send another PR shortly.

@igorw
Copy link
Contributor
igorw commented Apr 12, 2013

Looking at the PHP source this actually appears to be highly platform specific. Maybe just skip the escaping if it matches [a-zA-Z0-9-]+.

@Seldaek Seldaek mentioned this pull request Apr 12, 2013
Merged
fabpot added a commit that referenced this pull request Apr 12, 2013
@fabpot
merged branch Seldaek/argv-tostring (PR #7657)
0c88f22 
This PR was merged into the master branch.

Discussion
----------

[Console] Input::__toString escaping fixes

Follow up to #7648, also includes a fix for StringInput to parse newlines and other whitespace chars properly instead of normalizing them all to spaces. It was kinda needed to test it properly, so I bundled both in one.

Commits
-------

93b1369 [Console] Fix StringInput parsing to accept newlines and tabs
8642b67 [Console] Fix escaping of args
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Seldaek @igorw @fabpot @GromNaN
0