Skip to content

[Process] Allow writing portable "prepared" command lines #32126

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 26, 2019
Merged

[Process] Allow writing portable "prepared" command lines #32126

merged 1 commit into from
Jun 26, 2019

Conversation

Simperfit
Copy link
Contributor

@Simperfit Simperfit commented Jun 21, 2019 •
edited by nicolas-grekas
Loading

Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #23778
License MIT
Doc PR symfony/symfony-docs#11802

Hey here, it's me, again !

I've talked with @nicolas-grekas and he gave me a new way of writing this feature that will not be a problem for people using things like {{ toto }} since we are using the linux style of using envvar, the replaced arguments is only replaced when using windows.

This should not break anything and work as expected!

see #24763

This makes "$FOO" work seamlessly on Linux and on Windows to reference an env var (that can be provided when calling e.g. the "run" method)

@nicolas-grekas nicolas-grekas added this to the next milestone Jun 21, 2019
@Simperfit Simperfit force-pushed the feature/allow-writing-preprared-commandline branch from 3ff92f3 to f8c434d Compare June 21, 2019 07:29
@Simperfit Simperfit changed the title feature: use linux style and only replace windows [Process] Allow writing portable "prepared" command lines Jun 21, 2019
@Simperfit Simperfit force-pushed the feature/allow-writing-preprared-commandline branch 9 times, most recently from 3e2a402 to 0e31ca0 Compare June 22, 2019 16:20
nicolas-grekas
nicolas-grekas reviewed Jun 22, 2019
View reviewed changes
@Simperfit Simperfit force-pushed the feature/allow-writing-preprared-commandline branch from 0e31ca0 to 182693b Compare June 22, 2019 17:15
@Simperfit Simperfit force-pushed the feature/allow-writing-preprared-commandline branch 2 times, most recently from ff209ef to 0604cbb Compare June 22, 2019 17:57
@nicolas-grekas nicolas-grekas force-pushed the feature/allow-writing-preprared-commandline branch 3 times, most recently from 95c4422 to d72ce2b Compare June 23, 2019 16:03
nicolas-grekas
nicolas-grekas approved these changes Jun 23, 2019
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(once this last comment is fixed)

@Simperfit Simperfit force-pushed the feature/allow-writing-preprared-commandline branch from d72ce2b to 3f8354f Compare June 23, 2019 19:20
@fabpot
Copy link
Member

fabpot commented Jun 26, 2019

Thank you @Simperfit.

@fabpot fabpot merged commit 3f8354f into symfony:4.4 Jun 26, 2019
fabpot added a commit that referenced this pull request Jun 26, 2019
@fabpot
feature #32126 [Process] Allow writing portable "prepared" command li…
bd8ad3f 
…nes (Simperfit)

This PR was merged into the 4.4 branch.

Discussion
----------

[Process] Allow writing portable "prepared" command lines

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #23778  <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#11802 <!-- required for new features -->

Hey here, it's me, again !

I've talked with @nicolas-grekas and he gave me a new way of writing this feature that will not be a problem for people using things like {{ toto }} since we are using the linux style of using envvar, the replaced arguments is only replaced when using windows.

This should not break anything and work as expected!

see #24763

This makes `"$FOO"` work seamlessly on Linux and on Windows to reference an env var (that can be provided when calling e.g. the "run" method)

Commits
-------

3f8354f [Process] Allow writing portable "prepared" command lines
@Simperfit Simperfit deleted the feature/allow-writing-preprared-commandline branch June 28, 2019 10:31
@Simperfit Simperfit mentioned this pull request Aug 1, 2019
Closed
OskarStark added a commit to OskarStark/symfony-docs that referenced this pull request Aug 15, 2019
@OskarStark
feature symfony#11802 [Process] allow writing "prepared" command line…
a316cd5 
…. (Simperfit)

This PR was submitted for the master branch but it was merged into the 4.4 branch instead (closes symfony#11802).

Discussion
----------

[Process] allow writing "prepared" command line.

This documents the re-pushed prepared command line with a new style of writing it that does not break anything.

symfony/symfony#32126

Commits
-------

05f8941 [Process] allow writing "prepared" command line.
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
This was referenced Nov 12, 2019
Merged
Merged
@GuidoHendriks
Copy link

How's this not a breaking change? It suddenly decides to replace parts of the command. This breaks Laravel Valet: laravel/valet#810

@gmponos
Copy link
Contributor

gmponos commented Dec 3, 2019

We had this command on our project:

        $process = Process::fromShellCommandline('for file in $(ls -R translations/*.yml); do LC_COLLATE=C sort -u -o "$file" "$file"; done');
        $process->start();

although this is not a major BC for us I can imagine that for some ppl it is...

@stof
Copy link
Member

stof commented Dec 3, 2019

This is making fromShellCommandline unusable for writing non-portable code relying on bash-specific features creating variables inline indeed. @nicolas-grekas should we expose 2 different methods instead ?

@gmponos
Copy link
Contributor

gmponos commented Dec 3, 2019

FYI We fixed it with this:

$process = Process::fromShellCommandline('for file in $(ls -R translations/*.yml); do LC_COLLATE=C sort -u -o $file $file; done');

@nicolas-grekas
Copy link
Member

another option that preserves escaping would be:

'for file in $(ls -R translations/*.yml); do LC_COLLATE=C sort -u -o "${file}" "${file}"; done'

@fideloper
Copy link

fideloper commented Dec 5, 2019 •
edited
Loading

I have to agree that this is breaking. Feels grim to have my commands changed on me (and did, in fact, break on us).

A prime example is when your script creates and uses a variable:

FOO="whatever"
echo $FOO # this may raise an exception

Using the ${FOO} syntax worked great in our case, however does the ${FOO} syntax (to work around this) work on Windows?

I'm not sure I understand why this feature exists. I looked at #23778 but don't really see the benefit? (that's just to say it's not obvious to me, I'd love to see the use case to know more!)

@taylorotwell
Copy link
Contributor

We're also having similar problems with this change in laravel/envoy.

@driesvints
Copy link
Contributor

For Envoy: this will require everyone to adjust their scripts to use the new ${FOO} syntax. I also have no idea if that would work on Windows.

This was referenced Dec 5, 2019
Closed
Closed
@nicolas-grekas
Copy link
Member

No better idea than reverting on my side;

nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull request Dec 5, 2019
@nicolas-grekas
Revert "feature symfony#32126 [Process] Allow writing portable "prepa…
fb31366 
…red" command lines (Simperfit)"

This reverts commit bd8ad3f, reversing
changes made to f15722d.
@nicolas-grekas
Copy link
Member

Revert proposed in #34845

@driesvints
Copy link
Contributor

Thank you for this @nicolas-grekas! And thanks for all your hard work on Symfony :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Feature Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
10 participants
@Simperfit @fabpot @GuidoHendriks @gmponos @stof @nicolas-grekas @fideloper @taylorotwell @driesvints @carsonbot