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

Merged
merged 1 commit into from May 22, 2014

Projects

None yet

3 participants

@romainneutron
Member
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
Member

Deprecation is proposed in PR #10930

@stof
Member
stof commented May 17, 2014

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

@romainneutron
Member

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

@stof
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
Member

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

@romainneutron romainneutron changed the title from [Process] Add validation on Process input to [2.3][Process] Add validation on Process input May 17, 2014
@stof
Member
stof commented May 17, 2014

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

@stof
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
Member

PR updated.
I added more tests

@romainneutron romainneutron [Process] Add validation on Process input
583092b
@fabpot
Member
fabpot commented May 22, 2014

Thank you @romainneutron.

@fabpot fabpot merged commit 583092b into symfony:2.3 May 22, 2014

2 checks passed

continuous-integration/travis-ci The Travis CI build passed
Details
default Success: Travis, fabbot
Details
@fabpot fabpot added a commit that referenced this pull request May 22, 2014
@fabpot fabpot bug #10929 [2.3][Process] Add validation on Process input (romainneut…
…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
c8476ee
@romainneutron romainneutron deleted the romainneutron:stdin-values branch May 22, 2014
@fabpot fabpot added a commit that referenced this pull request May 23, 2014
@fabpot fabpot feature #10930 [Process] Deprecate using values that are not string f…
…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
bd68412
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment