ProcessBuilder clean up #3381

Seldaek · 2012-02-16T15:52:39Z

Code cleanup
Added create() static method for easy creation until we can do $process = (new ProcessBuilder())->add()->getProcess();
Removed windows wrapping of commands. This does not belong there IMO. If assetic needs that it should add it, and if it's generally beneficial to everyone then we should add it to Process, but having it implicitly only when using ProcessBuilder makes on sense.

…face until 5.4

… handling

beberlei · 2012-02-16T16:10:15Z

I agree on the windows stuff. I know it fixes a bunch of issues in Assetic, but it also caused my tons of headaches in my windows commands that didnt need strict escaping. Also this messes with parameters in Powershell for example, when you have "foo /bar:baz" then it makes this to ""foo" "/bar:baz"" which in some circumstances fails. Its all messy.

schmittjoh · 2012-02-16T17:53:30Z

Can you move the wrapping to the Process class instead? It's generally causing no bad side effects, but fixes a few issues in the proc_open implementation. It is also necessary for Assetic, and potentially other tools to work on Windows.

Seldaek · 2012-02-16T17:56:02Z

Sure, although "generally" sounds a bit scary in your sentence :)

What about the bypass_shell option?

schmittjoh · 2012-02-16T18:02:12Z

"generally" means I don't know of any, but what I do know is that the alternative you are suggesting is not working. Have there been any bug reports on Assetic/symfony/your own code that "cmd" wrapping causes problems?

Seldaek · 2012-02-16T18:04:59Z

No no, don't get me wrong, I'm not suggesting this should be removed. I'm just saying it should be done for all processes or none, but not just for those run via the ProcessBuilder because that's a good recipe for WTFs.

schmittjoh · 2012-02-16T18:09:38Z

Yeah, I understand, and it makes sense.

What I would suggest is to move it to the process class, and let a wider audience test this to see if we get any bug reports on strange behavior etc.

Seldaek · 2012-02-16T18:12:00Z

Still not sure about the bypass_shell option though. And @beberlei mentioned problems? Can you expand on that?

Seldaek · 2012-02-16T18:16:34Z

Added back to Process, with a switch so if anyone runs into problems they can easily disable it.

schmittjoh · 2012-02-22T04:52:44Z

src/Symfony/Component/Process/Process.php

@@ -34,6 +34,7 @@ class Process
    private $status;
    private $stdout;
    private $stderr;
+    private $enhanceWindowsCompatibility;


I think = true; is missing here. It's turned off by default atm, no?

Correct, fixed now.

Seldaek · 2012-02-22T10:59:58Z

Ping @fabpot - I think this is ready now
Ping @kriswallsmith if this gets merged please update Assetic stuff to restore the bypass_shell option if it's really needed.

kriswallsmith · 2012-02-22T12:41:15Z

Posting a PR under "code cleanup" that tinkers with a class that is inherently difficult to test for regression and has been tested by the community for over a year is… a bit hard to swallow, honestly. Everything is there for a reason and should not be tinkered with lightly.

For example, it's important that the $env variable default to null so the current environment is inherited by default — why change that?

I don't know what the bypass_shell option does, but @pierrejoye does… which is why he put it there.

I'm okay with adding an "enhanced Windows compatibility" switch, but I personally think is should be on the builder, not Process. The builder is where we manipulate the strings that compose the command line, not in Process. You're introducing manipulation of the command line to Process, which blurs the responsibilities of the two classes.

I'm also okay with the static factory method :)

Seldaek · 2012-02-22T13:19:40Z

@kriswallsmith (Sorry about the confusing title) My concern is just that if you use Process then decide to "upgrade" to the ProcessBuilder, you suddenly have a change of behavior that might break stuff without you noticing. I just want to avoid this unexpected behavior.

As for the $env stuff, I added a couple tests now, and then expanded that ternary operator a bit.. It actually was broken before. It passed null if you had no env set, but even if you did not call inheritEnvironmentVariables. If you want to inherit by default - which I agree it should - then why was inheritEnv = false in the constructor? I changed it too and now there is hopefully less confusion.

Restored bypass_shell=true unless it's explicitly set to false.

kriswallsmith · 2012-02-22T13:24:08Z

src/Symfony/Component/Process/ProcessBuilder.php


-            $script = 'cmd /V:ON /E:ON /C "'.$script.'"';
+        $env = null;


This is not necessary.

kriswallsmith · 2012-02-22T13:25:23Z

We should also add the PHPUnit @backupGlobals enabled annotation while we're in here.

kriswallsmith · 2012-02-22T13:28:25Z

src/Symfony/Component/Process/Process.php

@@ -115,7 +116,7 @@ public function __construct($commandline, $cwd = null, array $env = null, $stdin
        }
        $this->stdin = $stdin;
        $this->timeout = $timeout;
-        $this->options = array_merge(array('suppress_errors' => true, 'binary_pipes' => true, 'bypass_shell' => false), $options);
+        $this->options = array_merge(array('suppress_errors' => true, 'binary_pipes' => true), $options);


Let's change to array_replace().

kriswallsmith · 2012-02-22T13:31:41Z

@Seldaek Looks better, thanks for the changes. If enhanceWindowsCompatibility is going to live on Process we should expose the switch on the builder as well. Speaking of enhanceWindowsCompatibility… is there a more descriptive name for that? What exactly does that do, any why would anyone want to switch it off? The name is so vague we might as well call it enableMagicalWindowsFix().

pierrejoye · 2012-02-22T13:33:55Z

I really do not think that having a flag to enable portability is a
good idea, at all.

I do not remember the context right now but a flag is definitively a
bad idea (you will need other on other platforms).

I will take a look again at this next week (end of), as I am still OOF.

On Wed, Feb 22, 2012 at 2:31 PM, Kris Wallsmith
reply@reply.github.com
wrote:

@Seldaek Looks better, thanks for the changes. If enhanceWindowsCompatibility is going to live on Process we should expose the switch on the builder as well. Speaking of enhanceWindowsCompatibility… is there a more descriptive name for that? What exactly does that do, any why would anyone want to switch it off? The name is so vague we might as well call it enableMagicalWindowsFix().

Reply to this email directly or view it on GitHub:
#3381 (comment)

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

Seldaek · 2012-02-22T13:42:56Z

backupGlobals seems to be enabled by default.

As for the enhanceWindowsCompatibility, yes. It's a poor name, but no I don't have any idea for a better one, because nobody could explain me what it does. People just scream that it's necessary.

@pierrejoye: If you or anyone can conclusively confirm that this stuff is always better, then we always do it. If it's not then it must be optional, and if it's not a flag then what? The point of the component is to abstract the proc_open horrors. If people have to know about windows quirks with regard to proc_open to use it, then it's not a very useful abstraction.

Additionally, if it is always better to use those portability fixes, then why isn't php doing it itself?

pierrejoye · 2012-02-22T13:47:02Z

On Wed, Feb 22, 2012 at 2:42 PM, Jordi Boggiano
reply@reply.github.com
wrote:

backupGlobals seems to be enabled by default.

As for the enhanceWindowsCompatibility, yes. It's a poor name, but no I don't have any idea for a better one, because nobody could explain me what it does. People just scream that it's necessary.

@pierrejoye: If you or anyone can conclusively confirm that this stuff is always better, then we always do it. If it's not then it must be optional, and if it's not a flag then what? The point of the component is to abstract the proc_open horrors. If people have to know about windows quirks with regard to proc_open to use it, then it's not a very useful abstraction.

proc_open has many quirks, not only on windows. That's why it should
work and detect what is needed, that may force you to slightly change
the split between builder and process.

Additionally, if it is always better to use those portability fixes, then why isn't php doing it itself?

BC, like it or not (I do not).

However we cannot change past versions, so today code has to deal it
with it anyway.

I will take a look at what you are trying to fix here next week, if
you have any other requests regarding proc_open&portability, let me
know :)

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

Seldaek · 2012-02-22T13:54:38Z

Ok so it sounds to me like the current code is correct, it tries to fix
things as best as we know how to by default, and just gives you a way to
disable things in the odd case we messed up and some of those fixes are
harmful to some use cases.

fabpot · 2012-03-02T21:38:18Z

@Seldaek @kriswallsmith is it ready for merge now?

kriswallsmith · 2012-03-02T21:42:22Z

I'm still not happy with the name of enhanceWindowsCompatibility. We need to be more specific about what that does. It sounds like a marketing term right now ;)

Seldaek · 2012-03-05T13:44:56Z

Agreed, but I can't think of anything better. It is indeed esoteric magic fixes that should work better but nobody seems 100% sure about it, so I think it's fairly accurate.

@beberlei

Commits ------- 7444fdf Feedback fixes 54cfd44 Restore bypass_shell by default with windows compat 38df47a Fix env inheritance and added tests f555c62 [Process] Add windows compatibility to Process component c4e8ff7 [Process] Always escape commands properly and remove windows-specific handling 9e237f6 [Process] Add ProcessBuilder::create() for more fluidity in the interface until 5.4 4882777 [Process] Code clean up Discussion ---------- ProcessBuilder clean up - Code cleanup - Added create() static method for easy creation until we can do `$process = (new ProcessBuilder())->add()->getProcess();` - Removed windows wrapping of commands. This does not belong there IMO. If assetic needs that it should add it, and if it's generally beneficial to everyone then we should add it to Process, but having it implicitly only when using ProcessBuilder makes on sense. --------------------------------------------------------------------------- by beberlei at 2012-02-16T16:10:15Z I agree on the windows stuff. I know it fixes a bunch of issues in Assetic, but it also caused my tons of headaches in my windows commands that didnt need strict escaping. Also this messes with parameters in Powershell for example, when you have "foo /bar:baz" then it makes this to ""foo" "/bar:baz"" which in some circumstances fails. Its all messy. --------------------------------------------------------------------------- by schmittjoh at 2012-02-16T17:53:30Z Can you move the wrapping to the Process class instead? It's generally causing no bad side effects, but fixes a few issues in the proc_open implementation. It is also necessary for Assetic, and potentially other tools to work on Windows. --------------------------------------------------------------------------- by Seldaek at 2012-02-16T17:56:02Z Sure, although "generally" sounds a bit scary in your sentence :) What about the bypass_shell option? --------------------------------------------------------------------------- by schmittjoh at 2012-02-16T18:02:12Z "generally" means I don't know of any, but what I do know is that the alternative you are suggesting is not working. Have there been any bug reports on Assetic/symfony/your own code that "cmd" wrapping causes problems? --------------------------------------------------------------------------- by Seldaek at 2012-02-16T18:04:59Z No no, don't get me wrong, I'm not suggesting this should be removed. I'm just saying it should be done for all processes or none, but not just for those run via the ProcessBuilder because that's a good recipe for WTFs. --------------------------------------------------------------------------- by schmittjoh at 2012-02-16T18:09:38Z Yeah, I understand, and it makes sense. What I would suggest is to move it to the process class, and let a wider audience test this to see if we get any bug reports on strange behavior etc. --------------------------------------------------------------------------- by Seldaek at 2012-02-16T18:12:00Z Still not sure about the bypass_shell option though. And @beberlei mentioned problems? Can you expand on that? --------------------------------------------------------------------------- by Seldaek at 2012-02-16T18:16:34Z Added back to Process, with a switch so if anyone runs into problems they can easily disable it. --------------------------------------------------------------------------- by Seldaek at 2012-02-22T10:59:58Z Ping @fabpot - I think this is ready now Ping @kriswallsmith if this gets merged please update Assetic stuff to restore the bypass_shell option if it's really needed. --------------------------------------------------------------------------- by kriswallsmith at 2012-02-22T12:41:15Z Posting a PR under "code cleanup" that tinkers with a class that is inherently difficult to test for regression and has been tested by the community for over a year is… a bit hard to swallow, honestly. Everything is there for a reason and should not be tinkered with lightly. For example, it's important that the `$env` variable default to `null` so the current environment is inherited by default — why change that? I don't know what the `bypass_shell` option does, but @pierrejoye does… which is why he put it there. I'm okay with adding an "enhanced Windows compatibility" switch, but I personally think is should be on the builder, not `Process`. The builder is where we manipulate the strings that compose the command line, not in `Process`. You're introducing manipulation of the command line to `Process`, which blurs the responsibilities of the two classes. I'm also okay with the static factory method :) --------------------------------------------------------------------------- by Seldaek at 2012-02-22T13:19:40Z @kriswallsmith (Sorry about the confusing title) My concern is just that if you use Process then decide to "upgrade" to the ProcessBuilder, you suddenly have a change of behavior that might break stuff without you noticing. I just want to avoid this unexpected behavior. As for the $env stuff, I added a couple tests now, and then expanded that ternary operator a bit.. It actually was broken before. It passed null if you had no env set, but even if you did not call `inheritEnvironmentVariables`. If you want to inherit by default - which I agree it should - then why was `inheritEnv = false` in the constructor? I changed it too and now there is hopefully less confusion. Restored bypass_shell=true unless it's explicitly set to false. --------------------------------------------------------------------------- by kriswallsmith at 2012-02-22T13:25:23Z We should also add the PHPUnit `@backupGlobals enabled` annotation while we're in here. --------------------------------------------------------------------------- by kriswallsmith at 2012-02-22T13:31:41Z @Seldaek Looks better, thanks for the changes. If `enhanceWindowsCompatibility` is going to live on `Process` we should expose the switch on the builder as well. Speaking of `enhanceWindowsCompatibility`… is there a more descriptive name for that? What exactly does that do, any why would anyone want to switch it off? The name is so vague we might as well call it `enableMagicalWindowsFix()`. --------------------------------------------------------------------------- by pierrejoye at 2012-02-22T13:33:55Z I really do not think that having a flag to enable portability is a good idea, at all. I do not remember the context right now but a flag is definitively a bad idea (you will need other on other platforms). I will take a look again at this next week (end of), as I am still OOF. On Wed, Feb 22, 2012 at 2:31 PM, Kris Wallsmith <reply@reply.github.com> wrote: > @Seldaek Looks better, thanks for the changes. If `enhanceWindowsCompatibility` is going to live on `Process` we should expose the switch on the builder as well. Speaking of `enhanceWindowsCompatibility`… is there a more descriptive name for that? What exactly does that do, any why would anyone want to switch it off? The name is so vague we might as well call it `enableMagicalWindowsFix()`. > > --- > Reply to this email directly or view it on GitHub: > #3381 (comment) -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --------------------------------------------------------------------------- by Seldaek at 2012-02-22T13:42:56Z backupGlobals seems to be enabled by default. As for the enhanceWindowsCompatibility, yes. It's a poor name, but no I don't have any idea for a better one, because nobody could explain me what it does. People just scream that it's necessary. @pierrejoye: If you or anyone can conclusively confirm that this stuff is always better, then we always do it. If it's not then it must be optional, and if it's not a flag then what? The point of the component is to abstract the proc_open horrors. If people have to know about windows quirks with regard to proc_open to use it, then it's not a very useful abstraction. Additionally, if it *is* always better to use those portability fixes, then why isn't php doing it itself? --------------------------------------------------------------------------- by pierrejoye at 2012-02-22T13:47:02Z On Wed, Feb 22, 2012 at 2:42 PM, Jordi Boggiano <reply@reply.github.com> wrote: > backupGlobals seems to be enabled by default. > > As for the enhanceWindowsCompatibility, yes. It's a poor name, but no I don't have any idea for a better one, because nobody could explain me what it does. People just scream that it's necessary. > @pierrejoye: If you or anyone can conclusively confirm that this stuff is always better, then we always do it. If it's not then it must be optional, and if it's not a flag then what? The point of the component is to abstract the proc_open horrors. If people have to know about windows quirks with regard to proc_open to use it, then it's not a very useful abstraction. proc_open has many quirks, not only on windows. That's why it should work and detect what is needed, that may force you to slightly change the split between builder and process. > Additionally, if it *is* always better to use those portability fixes, then why isn't php doing it itself? BC, like it or not (I do not). However we cannot change past versions, so today code has to deal it with it anyway. I will take a look at what you are trying to fix here next week, if you have any other requests regarding proc_open&portability, let me know :) Cheers, -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --------------------------------------------------------------------------- by Seldaek at 2012-02-22T13:54:38Z Ok so it sounds to me like the current code is correct, it tries to fix things as best as we know how to by default, and just gives you a way to disable things in the odd case we messed up and some of those fixes are harmful to some use cases. --------------------------------------------------------------------------- by fabpot at 2012-03-02T21:38:18Z @Seldaek @kriswallsmith is it ready for merge now? --------------------------------------------------------------------------- by kriswallsmith at 2012-03-02T21:42:22Z I'm still not happy with the name of `enhanceWindowsCompatibility`. We need to be more specific about what that does. It sounds like a marketing term right now ;) --------------------------------------------------------------------------- by Seldaek at 2012-03-05T13:44:56Z Agreed, but I can't think of anything better. It is indeed esoteric magic fixes that should work better but nobody seems 100% sure about it, so I think it's fairly accurate.

Seldaek added 3 commits February 16, 2012 16:37

[Process] Code clean up

4882777

[Process] Add ProcessBuilder::create() for more fluidity in the inter…

9e237f6

…face until 5.4

[Process] Always escape commands properly and remove windows-specific…

c4e8ff7

… handling

schmittjoh reviewed Feb 22, 2012
View reviewed changes

[Process] Add windows compatibility to Process component

f555c62

Seldaek added 2 commits February 22, 2012 14:10

Fix env inheritance and added tests

38df47a

Restore bypass_shell by default with windows compat

54cfd44

kriswallsmith reviewed Feb 22, 2012
View reviewed changes

Feedback fixes

7444fdf

fabpot merged commit 7444fdf into symfony:master Mar 5, 2012

This was referenced Jan 18, 2017

[Process] Windows: stronger escaping, disabled delayed expansion, deprecated options #21254

Closed

[Process] Strong args escaping on Windows + deprecate compat settings #21347

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

ProcessBuilder clean up #3381

ProcessBuilder clean up #3381

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ProcessBuilder clean up #3381

ProcessBuilder clean up #3381

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Cheers,

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!