Added deprecation to cwd not existing Fixes #18249 #23708

alexbowers · 2017-07-28T21:12:55Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#18249
License	MIT
Doc PR	-

chalasr · 2017-07-28T21:20:58Z

src/Symfony/Component/Process/Tests/ProcessTest.php

+    /**
+     * @see https://github.com/symfony/symfony/issues/18249
+     */
+    public function testCwdParameterBugDeprecationIssue18249()


just testInvalidCwd

alexbowers · 2017-07-28T21:24:02Z

@chalasr I have renamed the test

chalasr · 2017-07-28T21:22:56Z

src/Symfony/Component/Process/Process.php

@@ -303,6 +303,10 @@ public function start(callable $callback = null, array $env = array())
            $ptsWorkaround = fopen(__FILE__, 'r');
        }

+        if (!file_exists($this->cwd)) {
+            trigger_error('The provided cwd does not exist. Command is currently ran against getcwd()', E_DEPRECATED);


there must be a @ in front of the function call and it should be an E_USER_DEPRECATED, the debug component handles them gracefully. See https://symfony.com/doc/current/components/phpunit_bridge.html#trigger-deprecation-notices

chalasr · 2017-07-28T21:25:53Z

src/Symfony/Component/Process/Tests/ProcessTest.php

+            $cmd = new Process('touch testing2.txt', __DIR__.'/notfound/');
+            $this->assertEquals(0, $cmd->run());
+        } catch (\Throwable $e) {
+            $this->assertTrue(true); // A deprecated error was thrown


nothing ensures that this assertion is reached.
Once the trigger_error call fixed, what you need is to just call run() (removing the try-catch around) and add as much @expectedDeprecation [expectedMessage] annotations as you need in the phpdoc bloc (https://symfony.com/doc/current/components/phpunit_bridge.html#write-assertions-about-deprecations)

This has been done

chalasr · 2017-07-28T22:18:42Z

UPGRADE-3.4.md

@@ -68,6 +68,9 @@ Process

 * The `Symfony\Component\Process\ProcessBuilder` class has been deprecated,
   use the `Symfony\Component\Process\Process` class directly instead.
+
+ * The `Symfony\Component\Process\Process` __construct `cwd` argument will throw 
+   a deprecation warning on `run` if the folder does not exist.


we should replace the (silent) deprecation log by a real exception in 4.0, UPGRADE-4.0.md should mention that

Should I put that in the upgrade log before the exception is even implemented? Or should there be a separate PR for the exception?

this should target 3.4. The upgrade note should be added when deprecating the behavior, we take care of doing the change when merging it up to master (because master is 4.0, where BC layers are removed).

How do i change the target?

If i change it now, surely doesn't it put all of master onto 3.4 which isn't what we want.

You need to rebase your changes on top of 3.4 using git rebase and then you can change the base branch by clicking the "Edit" button on the top right of your PR.

Any chance you can give me the details on how to rebase?

Alternatively, I'm going away for the weekend. If somebody else wants to apply these as a patch to 3.4 instead of master, i'm more than happy for you to go ahead and do it.

https://git-scm.com/docs/git-rebase

I believe this has been done properly now.

fabpot · 2017-07-29T06:42:27Z

src/Symfony/Component/Process/Process.php

@@ -303,6 +303,10 @@ public function start(callable $callback = null, array $env = array())
            $ptsWorkaround = fopen(__FILE__, 'r');
        }

+        if (!file_exists($this->cwd)) {


Shoudn't it be is_dir()?

nicolas-grekas · 2017-07-30T15:30:06Z

src/Symfony/Component/Process/Tests/ProcessTest.php

@@ -47,6 +47,25 @@ protected function tearDown()
        }
    }

+    /**
+     * @see https://github.com/symfony/symfony/issues/18249


Usually we don't add such notes, we rely on the git history instead, should be removed.

nicolas-grekas · 2017-07-30T15:30:46Z

src/Symfony/Component/Process/Process.php

@@ -303,6 +303,10 @@ public function start(callable $callback = null, array $env = array())
            $ptsWorkaround = fopen(__FILE__, 'r');
        }

+        if (!file_exists($this->cwd)) {
+            @trigger_error('The provided cwd does not exist. Command is currently ran against getcwd()', E_USER_DEPRECATED);


the message should mention that an exception will be thrown in 4.0

oh, and the message should tell since which version this deprecation exist (see other deprecation messages in the code)

nicolas-grekas · 2017-07-30T15:32:43Z

src/Symfony/Component/Process/Tests/ProcessTest.php

+    public function testInvalidCwd()
+    {
+        // Check that it works fine if the CWD exists
+        $cmd = new Process('touch testing1.txt', __DIR__);


fails on Windows

This should hopefully be fixed now.

alexbowers · 2017-07-30T18:12:36Z

I'll fix the other issues either his evening or tomorrow, but I don't have access to a windows machine to test or find a working test. Thanks

…

On 30 Jul 2017, at 16:33, Nicolas Grekas ***@***.***> wrote: @nicolas-grekas commented on this pull request. In src/Symfony/Component/Process/Tests/ProcessTest.php: > @@ -47,6 +47,25 @@ protected function tearDown() } } + /** + * @see #18249 + * + * @group legacy + * @expectedDeprecation The provided cwd does not exist. Command is currently ran against getcwd() + */ + public function testInvalidCwd() + { + // Check that it works fine if the CWD exists + $cmd = new Process('touch testing1.txt', __DIR__); fails on Windows — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

nicolas-grekas · 2017-08-07T07:10:07Z

Status: needs work

alexbowers · 2017-08-07T09:52:06Z

@nicolas-grekas Any chance you can provide a fix for windows? I have no machine to test it on.

I will fix the other issues.

nicolas-grekas · 2017-08-16T16:55:29Z

It looks like Windows already fails when the cwd doesn't exist:
https://msdn.microsoft.com/en-us/library/ms681382%28v=vs.85%29.aspx#ERROR_DIRECTORY

So you can just throw an exception on Windows - and adjust the test accordingly.

nicolas-grekas · 2017-08-31T14:34:15Z

ping @alexbowers

alexbowers · 2017-08-31T14:41:32Z

Thanks, I'll try to get around to it this evening. Completely forgot about this.

alexbowers · 2017-09-02T02:22:27Z

I think all outstanding issues were resolved, can this be confirmed?

nicolas-grekas · 2017-09-02T06:16:18Z

src/Symfony/Component/Process/Process.php

@@ -334,7 +337,11 @@ public function start(callable $callback = null/*, array $env = array()*/)
            $ptsWorkaround = fopen(__FILE__, 'r');
        }

-        $this->process = proc_open($commandline, $descriptors, $this->processPipes->pipes, $this->cwd, $env, $this->options);


the $this->options property must be used, not doing so breaks windows badly

nicolas-grekas · 2017-09-02T06:16:54Z

src/Symfony/Component/Process/Tests/ProcessTest.php

+    public function testInvalidCwd()
+    {
+        // Check that it works fine if the CWD exists
+        $cmd = new Process('echo test > testing1.txt', __DIR__);


there is no need to pipe the output to a file

nicolas-grekas · 2017-09-02T06:18:15Z

src/Symfony/Component/Process/Tests/ProcessTest.php

+    {
+        // Check that it works fine if the CWD exists
+        $cmd = new Process('echo test > testing1.txt', __DIR__);
+        $this->assertEquals(0, $cmd->run());


should be assertSame - but to me this assertion is not needed: the fact that a process runs correctly when all is fine is already tested in other cases

nicolas-grekas · 2017-09-02T06:19:05Z

src/Symfony/Component/Process/Tests/ProcessTest.php

+        $this->assertEquals(0, $cmd->run());
+
+        $cmd = new Process('echo test > testing2.txt', __DIR__.'/notfound/');
+        $this->assertEquals(0, $cmd->run());


this fails on windows, where suppress_error does not make it fallback to getcwd (but fail instead)

How should I handle this?

I cannot test on windows, because I do not have a device to test it with.

alexbowers · 2017-09-02T22:13:55Z

@chalasr @fabpot @nicolas-grekas

I believe all changes have been implemented.

nicolas-grekas

almost good :)

nicolas-grekas · 2017-09-03T14:19:56Z

UPGRADE-3.4.md

@@ -68,6 +68,9 @@ Process

 * The `Symfony\Component\Process\ProcessBuilder` class has been deprecated,
   use the `Symfony\Component\Process\Process` class directly instead.
+
+ * The `Symfony\Component\Process\Process` __construct `cwd` argument will throw 


[...] constructor's $cwd argument [...]

nicolas-grekas · 2017-09-03T14:21:27Z

src/Symfony/Component/Process/Process.php

@@ -318,6 +318,7 @@ public function start(callable $callback = null/*, array $env = array()*/)
        } elseif (null !== $env) {
            @trigger_error('Not inheriting environment variables is deprecated since Symfony 3.3 and will always happen in 4.0. Set "Process::inheritEnvironmentVariables()" to true instead.', E_USER_DEPRECATED);
        }
+


should be reverted

nicolas-grekas · 2017-09-03T14:22:27Z

src/Symfony/Component/Process/Process.php

@@ -334,6 +335,10 @@ public function start(callable $callback = null/*, array $env = array()*/)
            $ptsWorkaround = fopen(__FILE__, 'r');
        }

+        if (!is_dir($this->cwd)) {
+            @trigger_error('The provided cwd does not exist. Command is currently ran against getcwd(). This behaviour is deprecated since version 3.4 and will be removed in 4.0.', E_USER_DEPRECATED);


I think we should stick to my previous suggestion: on Windows, we should throw an exception instead of a deprecation, because on Windows, it's already broken, the getcwd fallback does not apply.

nicolas-grekas · 2017-09-03T14:23:18Z

src/Symfony/Component/Process/Tests/ProcessTest.php

+     */
+    public function testInvalidCwd()
+    {
+        if (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') {


should be if ('\\' === DIRECTORY_SEPARATOR) { to test for Windows, as everywhere else in the code base.

nicolas-grekas · 2017-09-03T14:23:54Z

src/Symfony/Component/Process/Tests/ProcessTest.php

+    /**
+     * @group legacy
+     * @expectedDeprecation The provided cwd does not exist. Command is currently ran against getcwd(). This behaviour is deprecated since version 3.4 and will be removed in 4.0.
+     * @expectedException \PHPUnit_Framework_Error_Warning


should be replaced by an @expectedException+@expectedExceptionMessage as suggested above

xabbuh · 2017-09-04T07:59:11Z

UPGRADE-3.4.md

@@ -69,6 +69,9 @@ Process
 * The `Symfony\Component\Process\ProcessBuilder` class has been deprecated,
   use the `Symfony\Component\Process\Process` class directly instead.

+ * The `Symfony\Component\Process\Process` constructor's $cwd argument will throw 


$cwd should also be enclosed with backticks

I think we should actually reword this (please also add the same entry to the CHANGELOG.md file of the Process component):

Passing a not existing working directory to the constructor of the Symfony\Component\Process\Process class is deprecated and will not be supported anymore in Symfony 4.0.

And please also add a similar entry to the UPGRADE-4.0.md file:

Passing a not existing working directory to the constructor of the Symfony\Component\Process\Process class is not supported anymore.

Are these messages verbatim?

xabbuh · 2017-09-04T07:59:35Z

src/Symfony/Component/Process/Process.php

+                throw new RuntimeException('The provided cwd does not exist.');
+            }
+
+            @trigger_error('The provided cwd does not exist. Command is currently ran against getcwd(). This behaviour is deprecated since version 3.4 and will be removed in 4.0.', E_USER_DEPRECATED);


Does Symfony follow british or american spelling across the board?

Ok, ill make that change this evening

alexbowers · 2017-09-07T22:42:43Z

@fabpot @nicolas-grekas @chalasr

Ready for review.

alexbowers · 2017-09-14T08:11:13Z

@fabpot do I resolve these conflicts, or do they get resolved upstream?

stof · 2017-09-14T13:13:16Z

src/Symfony/Component/Process/Tests/ProcessTest.php

+            $this->markTestSkipped('Unix handles this automatically.');
+        }
+
+        // Check that it works fine if the CWD exists


This does not test anything. To test that it works, it must be a separate test.

If the exception happens in the first process rather than the second one, the @expectedException assertion will be fine with it.

Handled this by forcing a fail if any exception occurs in that first section.

chalasr · 2017-09-14T18:24:36Z

src/Symfony/Component/Process/Tests/ProcessTest.php

@@ -1494,7 +1534,7 @@ public function testRawCommandLine()
 (
    [0] => -
    [1] => a
-    [2] => 
+    [2] =>


should be reverted (breaks tests)

chalasr · 2017-09-14T18:43:51Z

src/Symfony/Component/Process/CHANGELOG.md

@@ -5,6 +5,7 @@ CHANGELOG
 -----

 * deprecated the ProcessBuilder class
+ * Passing a not existing working directory to the constructor of the `Symfony\Component\Process\Process` class is deprecated and will not be supported anymore in Symfony 4.0.


* deprecated calling `Process::start()` without setting a valid working directory beforehand (via `setWorkingDirectory()` or constructor)

chalasr · 2017-09-14T18:44:02Z

UPGRADE-3.4.md

@@ -210,6 +210,8 @@ Process
 * The `Symfony\Component\Process\ProcessBuilder` class has been deprecated,
   use the `Symfony\Component\Process\Process` class directly instead.

+ * Passing a not existing working directory to the constructor of the `Symfony\Component\Process\Process` class is deprecated and will not be supported anymore in Symfony 4.0.


Actually Process::__construct() allows null for $cwd (and fills the property using getcwd()), allowing to call setWorkingDirectory() later. Unless we want to make it non nullable, this should talk about start(), not the constructor.
I propose:

Calling `Process::start()` without setting a valid working directory (via `setWorkingDirectory()` or constructor) beforehand is deprecated and will throw an exception in 4.0.

nicolas-grekas · 2017-09-26T11:57:23Z

@alexbowers anytime this week to fix the remaining comments?

alexbowers · 2017-09-26T12:08:21Z

Yes, I'll put it in my todo list.

…

On 26 September 2017 at 12:57, Nicolas Grekas ***@***.***> wrote: @alexbowers <https://github.com/alexbowers> anytime this week to fix the remaining comments? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23708 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAzc3qPIk28oSPfYbjQnhmGXqIDNGgozks5smOa5gaJpZM4OnFmq> .

nicolas-grekas · 2017-10-05T16:33:34Z

@alexbowers don't forget this PR, we're almost at it and it has to be in 3.4 :)

alexbowers · 2017-10-05T21:21:15Z

@nicolas-grekas Sent the patch again.

nicolas-grekas · 2017-10-05T22:17:29Z

Can you just apply fabbot's patch please?

alexbowers · 2017-10-05T22:19:11Z

Done, though that is for changes not related to anything i've done as far as I can tell.

fabpot · 2017-10-05T22:24:45Z

Thank you @alexbowers.

…exbowers) This PR was squashed before being merged into the 3.4 branch (closes #23708). Discussion ---------- Added deprecation to cwd not existing Fixes #18249 | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? |no  | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #18249 | License | MIT | Doc PR | - Commits ------- 261eae9 Added deprecation to cwd not existing Fixes #18249

alexbowers · 2017-10-05T22:29:10Z

🍾 Only took me 69 days to actually get around to doing this.

fabpot · 2017-10-05T22:30:26Z

Congrats! And thank you for your work and persistence @alexbowers, this is much appreciated.

…(xabbuh) This PR was merged into the 4.0-dev branch. Discussion ---------- [Process] drop non-existent working directory support | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23708 | License | MIT | Doc PR | Commits ------- ade7c90 drop non-existent working directory support

carsonbot added Status: Needs Review Deprecation labels Jul 28, 2017

chalasr reviewed Jul 28, 2017

View reviewed changes

chalasr requested changes Jul 28, 2017

View reviewed changes

fabpot requested changes Jul 29, 2017

View reviewed changes

chalasr added this to the 3.4 milestone Jul 29, 2017

nicolas-grekas reviewed Jul 30, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Aug 7, 2017

alexbowers changed the base branch from master to 3.4 September 2, 2017 02:03

nicolas-grekas reviewed Sep 2, 2017

View reviewed changes

nicolas-grekas reviewed Sep 3, 2017

View reviewed changes

nicolas-grekas approved these changes Sep 3, 2017

View reviewed changes

nicolas-grekas added Status: Reviewed and removed Status: Needs Work labels Sep 4, 2017

xabbuh reviewed Sep 4, 2017

View reviewed changes

xabbuh approved these changes Sep 14, 2017

View reviewed changes

stof requested changes Sep 14, 2017

View reviewed changes

Handle test that should pass, failing, better

29ad993

chalasr requested changes Sep 14, 2017

View reviewed changes

chalasr added Status: Needs Work and removed Status: Reviewed labels Sep 26, 2017

Improvement fixes

8e9ddd2

carsonbot added Status: Reviewed and removed Status: Needs Work labels Oct 5, 2017

chalasr approved these changes Oct 5, 2017

View reviewed changes

nicolas-grekas approved these changes Oct 5, 2017

View reviewed changes

Ran fabbot

f46239d

fabpot approved these changes Oct 5, 2017

View reviewed changes

fabpot closed this Oct 5, 2017

alexbowers deleted the issue/process-cwd-bug branch October 5, 2017 22:29

xabbuh mentioned this pull request Oct 6, 2017

[Process] drop non-existent working directory support #24467

Merged

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

greg-1-anderson mentioned this pull request Dec 11, 2017

Consider reverting Windows-only backwards compatibility break in Process 3.4 #25446

Closed

Added deprecation to cwd not existing Fixes #18249 #23708

Added deprecation to cwd not existing Fixes #18249 #23708

Conversation

alexbowers commented Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

alexbowers commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexbowers commented Jul 30, 2017 via email

nicolas-grekas commented Aug 7, 2017

alexbowers commented Aug 7, 2017

nicolas-grekas commented Aug 16, 2017

nicolas-grekas commented Aug 31, 2017

alexbowers commented Aug 31, 2017

alexbowers commented Sep 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Sep 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexbowers commented Sep 2, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexbowers commented Sep 7, 2017

alexbowers commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Sep 26, 2017

alexbowers commented Sep 26, 2017 via email

nicolas-grekas commented Oct 5, 2017

alexbowers commented Oct 5, 2017

nicolas-grekas commented Oct 5, 2017

alexbowers commented Oct 5, 2017

fabpot commented Oct 5, 2017

alexbowers commented Jul 28, 2017 •

edited

Loading

chalasr Jul 28, 2017 •

edited

Loading

robfrawley Jul 28, 2017 •

edited

Loading

nicolas-grekas Sep 2, 2017 •

edited

Loading

chalasr Sep 14, 2017 •

edited

Loading