Skip to content

[2.3][Process] Add validation on Process input #10929

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
May 22, 2014
Merged

[2.3][Process] Add validation on Process input #10929

merged 1 commit into from
May 22, 2014

Conversation

romainneutron
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT

This adds validation on Process input. For the moment, passing a stream would result in a PHP error.
I propose to deprecate values that are not strictly string in 2.6 (see upcoming PR)

@romainneutron romainneutron mentioned this pull request May 17, 2014
Merged
@romainneutron
Copy link
Contributor Author

Deprecation is proposed in PR #10930

@stof
Copy link
Member

stof commented May 17, 2014

The phpdoc documents the type of the argument as string. So passing a steam is not supported.

@romainneutron
Copy link
Contributor Author

That's why I added a check. Passing a stream with the current implementation results in an error

@stof
Copy link
Member

stof commented May 17, 2014

@romainneutron I would just cast the value as string when it is not null, without adding extra exceptions. People passing an object in it are misusing Symfony anyway, so the PHP error does the work too.
If we start using exceptions to validate string inputs being real strings, it would have to be done on the whole framework, and it is not worth it IMO. Even Hack does not go this way. The static analysis is strict about validating input, but the HHVM runtime let things pass through (well, not sure about the way scalar typehints are handled at runtime though, but other types like shapes are not validated at runtime)

@romainneutron
Copy link
Contributor Author

ok, let's cast scalars, throw exceptions for others type? See http://3v4l.org/6HTo5

@romainneutron romainneutron changed the title [Process] Add validation on Process input [2.3][Process] Add validation on Process input May 17, 2014
@stof
Copy link
Member

stof commented May 17, 2014

arf, I forgot resources are castable as string. So yeah, this case need to be validated

@stof
Copy link
Member

stof commented May 17, 2014

btw, the PHP 4.3.3+ error message is funny in your snippet: Warning: fopen(php://memory): failed to open stream: Success

@romainneutron
Copy link
Contributor Author

PR updated.
I added more tests

@fabpot
Copy link
Member

fabpot commented May 22, 2014

Thank you @romainneutron.

@fabpot fabpot merged commit 583092b into symfony:2.3 May 22, 2014
fabpot added a commit that referenced this pull request May 22, 2014
@fabpot
bug #10929 [2.3][Process] Add validation on Process input (romainneut…
c8476ee 
…ron)

This PR was merged into the 2.3 branch.

Discussion
----------

[2.3][Process] Add validation on Process input

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

This adds validation on Process input. For the moment, passing a stream would result in a PHP error.
I propose to deprecate values that are not strictly string in 2.6 (see upcoming PR)

Commits
-------

583092b [Process] Add validation on Process input
@romainneutron romainneutron deleted the stdin-values branch May 22, 2014 21:35
fabpot added a commit that referenced this pull request May 23, 2014
@fabpot
feature #10930 [Process] Deprecate using values that are not string f…
bd68412 
…or Process::setStdin and ProcessBuilder::setInput (romainneutron)

This PR was merged into the 2.4-dev branch.

Discussion
----------

[Process] Deprecate using values that are not string for Process::setStdin and ProcessBuilder::setInput

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

This deprecates passing a `Process` input any value that is not a strict string. This needs #10929 to be merged.
I don't know if the use of `trigger_error` is correct or should be removed.

Commits
-------

9887b83 [Process] Deprecate using values that are not string for Process::setStdin and ProcessBuilder::setInput
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.
3 participants
@romainneutron @stof @fabpot