Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Process] Silent behaviour change if folder doesn't exist #18249

Closed
shopblocks opened this issue Mar 21, 2016 · 9 comments · Fixed by #24612
Closed

[Process] Silent behaviour change if folder doesn't exist #18249

shopblocks opened this issue Mar 21, 2016 · 9 comments · Fixed by #24612

Comments

@shopblocks
Copy link

In Process, the second parameter is the cwd. If this folder doesn't exist, it seems to run a getcwd.

This should initiate / throw a warning or exception, since the behaviour change is / can be critical.

For us, we were performing git clone into a folder, but the folder doesn't exist, so it clones into the folder where console is ran from. This is on ~3.0

@linaori
Copy link
Contributor

linaori commented Mar 21, 2016

It's a bit weird that you can set the cwd via the constructor and via the setter. Both have slightly different behavior.

@jakzal
Copy link
Contributor

jakzal commented Mar 22, 2016

@shopblocks are you suggesting the behaviour was different in previous Symfony versions?

@shopblocks
Copy link
Author

Im afraid this is the first version I have tested this with, so do not know whether it is different to previous versions or not.

@jakzal
Copy link
Contributor

jakzal commented Mar 22, 2016

proc_open() won't change the working directory if it doesn't exist. It would be a BC break if we started throwing an exception.

@xabbuh
Copy link
Member

xabbuh commented May 11, 2016

We could trigger a deprecation in 3.2 when a non-existing directory is to be used as the working directory and throw an exception in 4.0.

@javiereguiluz javiereguiluz changed the title Silent behaviour change if folder doesnt exist [process] [Process] Silent behaviour change if folder doesn't exist Jan 18, 2017
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 28, 2017

@shopblocks would you like to submit a PR triggering that deprecation? (branch 3.4)

@alexbowers
Copy link
Contributor

I'm not sure what you mean by that. What is deprecated? The unexpected behaviour?

@nicolas-grekas
Copy link
Member

It's not deprecated yet, but the unexpected behavior could be yes.

@alexbowers
Copy link
Contributor

Okay, i'll take a look at this.

fabpot added a commit that referenced this issue Oct 5, 2017
…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 <!-- don't forget updating src/**/CHANGELOG.md files -->
| 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
@fabpot fabpot closed this as completed Oct 5, 2017
xabbuh added a commit that referenced this issue Oct 6, 2017
* 3.4: (26 commits)
  bumped Symfony version to 3.3.11
  updated VERSION for 3.3.10
  updated CHANGELOG for 3.3.10
  bumped Symfony version to 2.8.29
  updated VERSION for 2.8.28
  updated CHANGELOG for 2.8.28
  bumped Symfony version to 2.7.36
  updated VERSION for 2.7.35
  update CONTRIBUTORS for 2.7.35
  updated CHANGELOG for 2.7.35
  Added deprecation to cwd not existing Fixes #18249
  [Session] fix MongoDb session handler to gc all expired sessions
  Add changelog for deprecated DbalSessionHandler
  [Security] Look at headers for switch user username parameter
  Updated Test name and exception name to be more accurate
  newline at end of file
  changed exception message
  Ahh, I see.  It actually wants a newline!
  Removed newline
  Created new Exception to throw and modified tests.
  ...
This was referenced Oct 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants