Skip to content

Added deprecation to cwd not existing Fixes #18249 #23708

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

Closed
wants to merge 25 commits into from
Closed

Added deprecation to cwd not existing Fixes #18249 #23708

Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
a019543
Added deprecation to cwd not existing
alexbowers Jul 28, 2017
602a7d4
Upgrade Docs updated
alexbowers Jul 28, 2017
7103381
Fixed codestyle issues
alexbowers Jul 28, 2017
56a4a01
Renamed test
alexbowers Jul 28, 2017
f9225dd
Improved tests to check for deprecation properly
alexbowers Jul 28, 2017
4c9f3d0
Check if cwd is a directory, not a file
alexbowers Sep 2, 2017
f44bfd9
Remove unnecessary comment
alexbowers Sep 2, 2017
296cfe6
Improved documenting the deprecation
alexbowers Sep 2, 2017
5918f16
Improve error and make test pass again
alexbowers Sep 2, 2017
1dd14be
Use options properly
alexbowers Sep 2, 2017
06dd2eb
Run the commands without checking exit codes
alexbowers Sep 2, 2017
d684e70
Improve testing
alexbowers Sep 2, 2017
7e0141c
Forgot to put semi colon on
alexbowers Sep 2, 2017
1e4abf6
Fix test
alexbowers Sep 2, 2017
53b2c2e
Fixing coding style
alexbowers Sep 2, 2017
8830131
Windows exception
alexbowers Sep 3, 2017
d9746a2
Removed newline
alexbowers Sep 3, 2017
babbfea
Changed the upgrade doc wording
alexbowers Sep 3, 2017
d57ac4d
Codestyle fix
alexbowers Sep 3, 2017
b93e7ad
Fixes
alexbowers Sep 7, 2017
e8a2702
Behaviour -> behavior
alexbowers Sep 14, 2017
647fdff
New line
alexbowers Sep 14, 2017
29ad993
Handle test that should pass, failing, better
alexbowers Sep 14, 2017
8e9ddd2
Improvement fixes
alexbowers Oct 5, 2017
f46239d
Ran fabbot
alexbowers Oct 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Changed the upgrade doc wording
  • Loading branch information
@alexbowers
alexbowers committed Sep 14, 2017
commit babbfea10f22d57984882def41422b84708c3b51
4 changes: 2 additions & 2 deletions UPGRADE-3.4.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,8 @@ 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

* The `Symfony\Component\Process\Process` constructor's $cwd argument will throw
Copy link
Member

Choose a reason for hiding this comment

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

$cwd should also be enclosed with backticks

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are these messages verbatim?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

a deprecation warning on `run` if the folder does not exist.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@chalasr chalasr Jul 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@robfrawley robfrawley Jul 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this has been done properly now.


Profiler
Expand Down