Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Process] Deprecate Process::setStdin in favor of Process::setInput #10932

Merged
merged 1 commit into from May 22, 2014

Conversation

romainneutron
Copy link
Contributor

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

From the Process point of view, what we pass is an input, as well as we retrieve output and error output.
As we use getOutput and getErrorOutput we should use setInput and let the underlying ProcessPipes deal with the actual STDIN.

By the way, ProcessBuilder already has setInput method and no setStdin method

@@ -1038,10 +1038,22 @@ public function setEnv(array $env)
* Gets the contents of STDIN.
*
* @return string|null The current contents
*
* @deprecated As of Symfony 2.6 This method is deprecated in favor of setInput
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: getInput

@romainneutron romainneutron changed the title [Process] Deprecate Process::setStdin in favor if Process::setInput [Process] Deprecate Process::setStdin in favor of Process::setInput May 18, 2014
@romainneutron
Copy link
Contributor Author

PR updated, typo fixed

@@ -224,7 +224,7 @@ public function mustRun($callback = null)
}

/**
* Starts the process and returns after sending the STDIN.
* Starts the process and returns after writing the input to STDIN.
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not always write to stdin anymore, does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If an input is passed, yes it's written to the underlying process's stdin.

@romainneutron
Copy link
Contributor Author

PR Updated, comments addressed


### Process

* Process::setStdin and Process::getStdin have been removed. Use
Copy link
Member

Choose a reason for hiding this comment

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

The method names should end with ()

@romainneutron
Copy link
Contributor Author

@fabpot updated, changed target to 2.5 instead of 2.6

@fabpot
Copy link
Member

fabpot commented May 22, 2014

Thanks @romainneutron.

@fabpot fabpot merged commit 53b9d73 into symfony:master May 22, 2014
fabpot added a commit that referenced this pull request May 22, 2014
…ess::setInput (romainneutron)

This PR was merged into the 2.3-dev branch.

Discussion
----------

[Process] Deprecate Process::setStdin in favor of Process::setInput

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

From the `Process` point of view, what we pass is an *input*, as well as we retrieve *output* and *error output*.
As we use `getOutput` and `getErrorOutput` we should use `setInput` and let the underlying `ProcessPipes` deal with the actual `STDIN`.

By the way, `ProcessBuilder` already has `setInput` method and no `setStdin` method

Commits
-------

53b9d73 [Process] Deprecate Process::setStdin in favor of Process::setInput
@darektw
Copy link

darektw commented May 22, 2014

it does not pass tests...

Failed asserting that exception message 
'Symfony\Component\Process\Process::setInput only accepts strings.' 
contains 
'Symfony\Component\Process\Process::setStdin only accepts strings.'.

please change tested exception messages!

@romainneutron
Copy link
Contributor Author

Thanks for the report @MarcomTeam. The issue has been introduced while merging 2.3 in master. A PR has been submitted, it should be fixed soon.

fabpot added a commit that referenced this pull request May 22, 2014
…recation (romainneutron)

This PR was merged into the 2.3-dev branch.

Discussion
----------

[Process] Fix conflicts between latest 2.3 fix and 2.5 deprecation

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

As reported in #10932 (comment), it's been introduced when merging 2.3 in master

Commits
-------

3454d60 [Process] Fix conflicts between latest 2.3 fix and 2.5 deprecation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants