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

[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

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedFeb 28, 2018
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR-

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.

@malarzm
Copy link
Contributor

malarzm commentedFeb 28, 2018
edited
Loading

How about an actual flag in__construct or a named constructor? IIRC! and!! had a meaning in shell. While their usage via Process component can be questioned, it is still changing original behaviour.

EDIT: also not knowing about the opt-in process I'd pull my hair out why the command is prefixed with a!.

jvasseur reacted with thumbs up emoji

@nicolas-grekas
Copy link
MemberAuthor

the constructor is already overloaded with arguments,
what meaning does! have? to me, it's only for interactive use.
can you give an example?

@malarzm
Copy link
Contributor

what meaning does ! have? to me, it's only for interactive use.

It might have been for interactive usage only,!! is rerunning last command, not sure if it requires interactive or not. But interactive or not, reading the command as a new comer I would wonder why shell command is prepended with a!.

the constructor is already overloaded with arguments,

Then we can go with a named constructor:Process::prepared($commandline, string $cwd = null, array $env = null, $input = null, ?float $timeout = 60) which would hide away the additional flag in the original__construct. Also this will clearly indicate that one is about to use prepared command.

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedFeb 28, 2018
edited
Loading

@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 eggit fetch git@github.com:symfony/symfony refs/pull/26344/head:prepared-process. About your comment for a named constructor, you'll also need to deal with theProcess::setCommandline() method.

$commandline ='exec'.$commandline;
}
}else {
}elseif ('!' ===substr($commandline =trim($commandline),0,1)) {
Copy link
Contributor

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?

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!

Copy link
MemberAuthor

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

It looks like enough people are interested into working on this and have ideas different than mines.
That's really great, no need for me here :)
As I'm on holiday next week, I won't be able to further work on this.
Please take over.

@nicolas-grekasnicolas-grekas deleted the process-prep-optin branchFebruary 28, 2018 19:33
@Simperfit
Copy link
Contributor

I’m taking over,@Tobion could you please crate an issue and ping me in it so we won’t forget ?

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedFeb 28, 2018
edited
Loading

Just one last note: don't be fooled, the feature at glance isnot about writing prepared command lines.
Prepared command lines are already possible since 3.3.

Here is one that works on 3.3:
(new Process('echo "$FOO"'))->run(null, array('FOO' => 123));

The feature at glance is theportability of prepared command lines, as on Windows, the same has to be written as:
(new Process('echo !FOO!'))->run(null, array('FOO' => 123));

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.

Simperfit reacted with thumbs up emoji

@malarzm
Copy link
Contributor

Sure, I'll try to prepare a PR tomorrow unless@Simperfit beats me to it ;)

fabpot added a commit that referenced this pull requestMar 5, 2018
…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)"
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@javiereguiluzjaviereguiluzjaviereguiluz left review comments

+1 more reviewer

@TobionTobionTobion left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

6 participants

@nicolas-grekas@malarzm@Simperfit@javiereguiluz@Tobion@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp