[Process] Make prepared command lines opt-in #26344
Conversation
|
How about an actual flag in EDIT: also not knowing about the opt-in process I'd pull my hair out why the command is prefixed with a |
|
the constructor is already overloaded with arguments, |
It might have been for interactive usage only,
Then we can go with a named constructor: |
|
@malarzm would you like to lead this effort? it feels like you might want to help :) If so, I invite you to borrow this PR and make the patch yours. Should be done using eg |
| @@ -269,7 +269,8 @@ public function start(callable $callback = null, array $env = array()) | ||
| // exec is mandatory to deal with sending a signal to the process | ||
| $commandline = 'exec '.$commandline; | ||
| } | ||
| } else { | ||
| } elseif ('!' === substr($commandline = trim($commandline), 0, 1)) { |
There was a problem hiding this comment.
why another magic thing instead of having a real api?
There was a problem hiding this comment.
I agree with @Tobion again. Although I don't think that ! is magic, it's another "Symfony thing" that users must learn about. It'd be better to add some builder process. I hope Tobias or any other contributor can take over this. Thanks!
There was a problem hiding this comment.
another API is also something people will need to learn, there is little difference, except verbosity maybe
but anyway, your topic now :)
|
It looks like enough people are interested into working on this and have ideas different than mines. |
|
I’m taking over, @Tobion could you please crate an issue and ping me in it so we won’t forget ? |
|
Just one last note: don't be fooled, the feature at glance is not about writing prepared command lines. Here is one that works on 3.3: The feature at glance is the portability of prepared command lines, as on Windows, the same has to be written as: That's why I resisted creating any new API (and I'm still dubious about the need for any): writing a portable prepared command line should be easier than using a ternary operator, which is all what is needed to achieve portability on 3.3. |
|
Sure, I'll try to prepare a PR tomorrow unless @Simperfit beats me to it ;) |
…e "prepared" command lines (Simperfit)" (nicolas-grekas) This PR was merged into the 4.1-dev branch. Discussion ---------- Revert "feature #24763 [Process] Allow writing portable "prepared" command lines (Simperfit)" | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This reverts commit 1364089, reversing changes made to e043478. As discussed in #24763 and #26344 This doens't revert the possibility to use prepared command lines. They just won't be *portable* anymore, unless special care is taken by "userland". Ie the placeholders need to be shell-dependent: use eg `echo "$FOO"` on *nix (the double quotes *are* important), and `echo !FOO!` on Windows (no double quotes there). Commits ------- 6a98bfa Revert "feature #24763 [Process] Allow writing portable "prepared" command lines (Simperfit)"
As discussed in #24763, prepared command lines should be opt-in.
Here is the way to opt-in: just prefix your command lines with a
!and this will make{{ FOO }}placeholders replaceable by values provided to either the constructor or therun()method.e.g.
(new Process('!echo {{ FOO }}'))->run(null, array('FOO' => 123));Btw, this should be the recommended way to run any command line IMHO.