Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Process] Make prepared command lines opt-in#26344
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
malarzm commentedFeb 28, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 |
nicolas-grekas commentedFeb 28, 2018
the constructor is already overloaded with arguments, |
malarzm commentedFeb 28, 2018
It might have been for interactive usage only,
Then we can go with a named constructor: |
nicolas-grekas commentedFeb 28, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 |
| $commandline ='exec'.$commandline; | ||
| } | ||
| }else { | ||
| }elseif ('!' ===substr($commandline =trim($commandline),0,1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why another magic thing instead of having a real api?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
another API is also something people will need to learn, there is little difference, except verbosity maybe
but anyway, your topic now :)
nicolas-grekas commentedFeb 28, 2018
It looks like enough people are interested into working on this and have ideas different than mines. |
Simperfit commentedFeb 28, 2018
I’m taking over,@Tobion could you please crate an issue and ping me in it so we won’t forget ? |
nicolas-grekas commentedFeb 28, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Just one last note: don't be fooled, the feature at glance isnot about writing prepared command lines. Here is one that works on 3.3: The feature at glance is theportability 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 aportable prepared command line should be easier than using a ternary operator, which is all what is needed to achieve portability on 3.3. |
malarzm commentedFeb 28, 2018
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 commit1364089, reversingchanges made toe043478.As discussed in#24763 and#26344This 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)"
Uh oh!
There was an error while loading.Please reload this page.
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.