Skip to content

[Process] Suppress warnings when open_basedir is non-empty #27141

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

Merged
merged 1 commit into from
May 15, 2018
Merged

[Process] Suppress warnings when open_basedir is non-empty #27141

merged 1 commit into from
May 15, 2018

Conversation

cbj4074
Copy link
Contributor

@cbj4074 cbj4074 commented May 3, 2018

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

If PHP is configured with a non-empty open_basedir value that does not permit access to the target location, these calls to is_executable() throw warnings.

While Symfony may not raise exceptions for warnings in production environments, other frameworks (such as Laravel) do, in which case any of these checks causes a show-stopping 500 error.

We fixed a similar issue in the ExecutableFinder class via #16182 .

This has always been an issue, but symfony/process@709e15e made it more likely that a warning is triggered.

If PHP is configured *with a non-empty open_basedir* value that does not permit access to the target location, these calls to is_executable() throw warnings.

While Symfony may not raise exceptions for warnings in production environments, other frameworks (such as Laravel) do, in which case any of these checks causes a show-stopping 500 error.

We fixed a similar issue in the ExecutableFinder class via #16182 .

This has always been an issue, but 709e15e7a37cb7ed6199548dc70dc33168e6cb2d made it more likely that a warning is triggered.
@nicolas-grekas nicolas-grekas added this to the 2.7 milestone May 3, 2018
@nicolas-grekas nicolas-grekas changed the title Suppress warnings when open_basedir is non-empty [Process] Suppress warnings when open_basedir is non-empty May 3, 2018
@xabbuh
Copy link
Member

xabbuh commented May 4, 2018

This was recently rejected in #26473. Do we want to revise that decision?

@stof
Copy link
Member

stof commented May 4, 2018

Well, currently, the PhpExecutableFinder is unusable with open_basedir. So I would say that this is one of the case where we should use @

@cbj4074
Copy link
Contributor Author

cbj4074 commented May 5, 2018

I appreciate the desire not to use @, like, ever, but what are the alternatives? Maybe users can live with them.

The alternatives seem to be:

  1. Don't use the PhpExecutableFinder component at all if open_basedir is non-empty. Users would have to go out of their way to avoid using this component, particularly in third-party use-cases, e.g., Laravel.
  2. Determine every file/directory that the executable finder could possibly query, and add all of those paths to the open_basedir directive. Is this practical on every system? Maybe... I haven't investigated it. And it seems that any environment variables configured on the system could affect which paths need to be added. Basically, it seems that users would need some type of Symfony-provided tool to scan the system, examine any environment variables, and print-out a string that needs to be added to open_basedir. Otherwise, it's guess-and-check and hope nothing breaks when deployed to other environments (let's be realistic, not everybody employs proper CI workflow).

I will say that I have mixed feelings about modifying my PHP configuration just to avoid warnings or errors that an arbitrary library will cause otherwise. In this specific instance, I'm not "purposely doing anything" with PhpExecutableFinder. Laravel seems to be using it for whatever purpose. Nothing seems to "break" if I simply suppress the calls (as in this PR), so its ability to find the php executable on my system seems not to be necessary in my specific case.

Whatever you all think... I'm not going to kick and scream. :) But I would like to know how best to determine which paths need to be added to open_basedir if ultimately this PR is not merged.

Thanks!

@chinawilon
Copy link

PhpExecutableFinder line:65

 if (is_executable($php = PHP_BINDIR.('\\' === DIRECTORY_SEPARATOR ? '\\php.exe' : '/php'))) {
            return $php;
 }   

is_executable() throw warnings when open_basedir is non-empty !

@nicolas-grekas
Copy link
Member

Thank you @cbj4074.

@nicolas-grekas nicolas-grekas merged commit 34f136e into symfony:2.7 May 15, 2018
nicolas-grekas added a commit that referenced this pull request May 15, 2018
… (cbj4074)

This PR was merged into the 2.7 branch.

Discussion
----------

[Process] Suppress warnings when open_basedir is non-empty

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

If PHP is configured *with a non-empty open_basedir* value that does not permit access to the target location, these calls to is_executable() throw warnings.

While Symfony may not raise exceptions for warnings in production environments, other frameworks (such as Laravel) do, in which case any of these checks causes a show-stopping 500 error.

We fixed a similar issue in the ExecutableFinder class via #16182 .

This has always been an issue, but symfony/process@709e15e made it more likely that a warning is triggered.

Commits
-------

34f136e Suppress warnings when open_basedir is non-empty
@cbj4074 cbj4074 deleted the patch-2 branch May 15, 2018 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants