Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:2.3fromromainneutron:stdin-values
May 22, 2014

Conversation

@romainneutron
Copy link
Contributor

QA
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsn/a
LicenseMIT

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
Copy link
ContributorAuthor

Deprecation is proposed in PR#10930

@stof
Copy link
Member

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

@romainneutron
Copy link
ContributorAuthor

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

@stof
Copy link
Member

@romainneutron I would just cast the value as string when it is notnull, 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
ContributorAuthor

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

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

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

@stof
Copy link
Member

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
ContributorAuthor

PR updated.
I added more tests

@fabpot
Copy link
Member

Thank you@romainneutron.

@fabpotfabpot merged commit583092b intosymfony:2.3May 22, 2014
fabpot added a commit that referenced this pull requestMay 22, 2014
…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       | MITThis 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
@romainneutronromainneutron deleted the stdin-values branchMay 22, 2014 21:35
fabpot added a commit that referenced this pull requestMay 23, 2014
…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       | MITThis 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 freeto 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
[8]ページ先頭
©2009-2025 Movatter.jp