8000 Process.php exception when $_ENV contains array value · Issue #7196 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

Process.php exception when $_ENV contains array value #7196

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

Jump to bottom
Closed
mmucklo opened this issue Feb 26, 2013 · 24 comments
Closed

Process.php exception when $_ENV contains array value #7196

mmucklo opened this issue Feb 26, 2013 · 24 comments
Labels
Process

Comments

@mmucklo
Copy link
Contributor
mmucklo commented Feb 26, 2013

Line 138-142 of Process.php:

        if (null !== $env) {
            $this->env = array();
            foreach ($env as $key => $value) {
                $this->env[(binary) $key] = (binary) $value;
            }

In certain circumstances, ProcessBuilder.php will grab $_ENV and merge it into the array passed into Process.php

ProcessBuilder.php line 148-154:

        if ($this->inheritEnv) {
            $env = $this->env ? $this->env + $_ENV : null;
        } else {
            $env = $this->env;
        }

        return new Process($script, $this->cwd, $env, $this->stdin, $this->timeout, $options);

The problem is that there are certain cases where $_ENV['something'] can be an array.

Right now in one of our environments, I'm seeing $_ENV['argv'] as an array, for example. This happens during Less to CSS generation.
@romainneutron romainneutron mentioned this issue Apr 26, 2013
Closed
@robclancy
Copy link

3 months open? I am getting this issue now.

@Pop-Code
Copy link

Hi, same issue for me now.

@beberlei
Copy link
Contributor

@robclancy how about trying to fix it instead of being rude? Symfony is an open source project and even if hundrets of people have contributed to far, you cannot ever fix every reported bug in a heartbeat.

@robclancy
Copy link

Trying to fix it? It's a simple fix anyone could make. It has been stated in another issue that it is "the devs fault for letting an array be in there". So now I have to find where said developer forced this in 2 dependencies deep and work out how to hack a fix around it because someone doesn't want to add an is_array call.

Also if you call that rude, well lol. I was wondering why it was 3 months open without a reply to say it isn't going to be fixed or otherwise. Instead I found that it infact isn't going to be fixed in another issue so this should be closed as "no fix, can't ! is_array because your fault".

@mmucklo
Copy link
Contributor Author
mmucklo commented May 17, 2013

@beberlei I've already submitted a pull request for this issue, but it was rejected. If someone would kindly show me which file should be patched instead, I'll happily submit another pull request.

Original Pull Request:
#7354

@robclancy
Copy link

That is what I was talking about... the "#7354 (comment)". All it needs is an array check, don't know why it is being refused. I have had to copy paste the class and have composers autoloader pick up the new class instead of the symfony one. Hacky but what choice do I have?

@Pop-Code
Copy link

I found that was my Mamp Env that was misconfigured.

If you use MAMP that adds strange vars to $_ENV, you can open this file:
/Applications/MAMP/Library/bin/envvars
and comment out the two lines for $DYLD_LIBRARY_PATH

After restarting server, everything was working again.

@heyflynn
Copy link

@AHWEBDEV : I started getting the same issue 3 days in dev environment on macs. your suggestion did not fix the problem unfortunately.

@oste
Copy link
oste commented May 20, 2013

I am seeing this error now after updating with composer. I think it is just the CSS to LESS conversion that is causing it.

I updated from symfony/symfony 2.2.x-dev (b4b4326 => f213a85)

@oste
Copy link
oste commented May 20, 2013

To be more specific I have a filter configured for assetic that looks like this.

assetic:
    filters:
        less:
            node:       /usr/local/bin/node
            node_paths: [/usr/local/lib/node, /usr/local/lib/node_modules]
            apply_to: "\.less$"

@mmucklo
Copy link
Contributor Author
mmucklo commented May 21, 2013

We too saw it during less compilation. There was something we did so that $_ENV{'argv'} wouldn't get set or something like that, which fixed the issue for the moment. Nevertheless, I think the class or it's caller should definitely address the issue in case it happens for the future.

@oste
Copy link
oste commented May 21, 2013

@mmucklo do you remember what it was that you did to take care of that $_ENV{'arg'}? I actually modified my envars file as @AHWEBDEV suggests here for a previous issue I was having so this is not a fix for me :(

For now I am using lessphp and unfortunately that does not meet all of my needs.

Ultimately it would be great to see a resolution for this issue, one way or the other.

@PaddyLock
Copy link

I'm getting the same issue since using composer to update with "symfony/symfony": "2.2.*",

ErrorException: Notice: Array to string conversion in vendor/symfony/symfony/src/Symfony/Component/Process/Process.php line 150

I am using sass and compass to compile css with default settings

assetic:
filters:
cssrewrite: ~
scss: ~
compass: ~

The line in MAMP envvars that @AHWEBDEV recommends to comment out is already commented out, so this doesn't fix it.

I also tries down grading to symfony 2.2.0 and the problem still exists.

@mmucklo
Copy link
Contributor Author
mmucklo commented May 21, 2013

Here's how we fixed the issue, in php.ini, we did this:

register_argc_argv = On

Before, it was:

register_argc_argv = Off

@oste
Copy link
oste commented May 21, 2013

Unfortunately I already have this setting configured as you have described. Also I tried to revert like @PaddyLock tried and I found it strange that I could not go back to a working state. I even tried removing all vendors and checking out a specific branch using the following requirement in composer "symfony/symfony": "2.2.x-dev#b4b4326a1016220e6240211d054cc3ecfadd74be"

@PaddyLock
Copy link

@mmucklo I checked my php.ini and I already have

register_argc_argv = On

The only way I could fix mine was to temporarily comment out vendor/symfony/symfony/src/Symfony/Component/Process/Process.php line 150

Not good practice I know, but it works for now.

@Pop-Code
Copy link

@PaddyLock , @mmucklo said to dactivate it, not to activate it.

try register_argc_argv = Off instead.

@PaddyLock
Copy link

@AHWEBDEV thanks, yes setting to Off works!

register_argc_argv = Off

I re-read the comment and it does sound like @mmucklo said to set to On, so a bit confused. @mmucklo ?

Does anyone know if anything else could break by turning this off? And is this a bug in Symfony2 that should be fixed?

Thanks

@Pop-Code
Copy link

The problem is that the proc_open function can only take flat array as $_ENV argument.
If register_argc_argv is turned on, this will add the $_ENV['argv'] that is an array.

Then the process will construct the $env propety that is used in the proc_open() function.
The fix would be to check if value populated to the property $env are not array and are only flat values.

But the symfony team seems do not want to fix that because they said that it is the dev role to be sure the $_ENV Global do not contains any arrays

A PR was submit to fix that as i said, but was refused.
So i think maybe a better verification in the symfony2 app/check.php for cli and in the config.php for frontend of the ENV configuration would do the job.
Adding some waring if the register_argc_argv = On.

But i am not enought aware of the impact of a change like this.

@robclancy
Copy link

This simply should be fixed in symfony. It is not our problem, it is a bug in symfony. A VERY easily fixed one that isn't to be fixed because "we just shouldn't do that".

I have already hacked in my workaround so don't care what happens much but it annoys me that such a big framework refuses to fix such a trivial bug.

@Pop-Code
Copy link

I 100% agree, Symfony team should fix that, at least make some verifications instead of populating the whole env.

@mmucklo
Copy link
Contributor Author
mmucklo commented May 23, 2013

Actually I just performed a full composer update of our symfony2.2 stack and I found that neither:

register_argc_argv = On

(the original fix we did), nor turning it back off:

register_argc_argv = Off

Are working anymore...Maybe I should just submit a patch for ProcessBuilder.php where it uses $_ENV? I've hesitated to do it as it might not be very pretty (maybe an array_filter or something right there could work).

This was referenced May 23, 2013
Closed
Closed
@mmucklo
Copy link
Contributor Author
mmucklo commented May 23, 2013

Sorry for the spam everyone - been a long night...Anyway I merged the patch into the 2.2 branch of my fork and it seems to be working for me when I point composer at it.

@mmucklo mmucklo mentioned this issue Jun 7, 2013
Closed
@mmucklo
Copy link
Contributor Author
mmucklo commented Jun 7, 2013

Just made a clean pull request against mast 8000 er.

@fabpot fabpot closed this as completed Jun 13, 2013
egeloen pushed a commit to egeloen/symfony that referenced this issue Nov 4, 2013
@mmucklo @fabpot
fix issue where $_ENV contains array vals
8764944 
There are cases where $env or $_ENV can contain a value that is an array
This will cause Process to throw an Array to String conversion exception
Initially I submitted a patch of Process.php, however Fabien indicated
that it shouldn't be fixed there (see below pull request).

Before recently, a simple work around would be in php.ini to set:

  register_argc_argv = On

However with recent changes, this seems to no longer work.

Original pull request: symfony#7354

See ticket symfony#7196
egeloen pushed a commit to egeloen/symfony that referenced this issue Nov 4, 2013
@fabpot
merged branch oodle/ticket_7196 (PR symfony#8227)
6089f32 
This PR was submitted for the master branch but it was merged into the 2.2 branch instead (closes symfony#8227).

Discussion
----------

[Process] fix issue where $_ENV contains array vals

There are cases where $env or $_ENV can contain a value that is an array
This will cause Process to throw an Array to String conversion exception
Initially I submitted a patch of Process.php, however Fabien indicated
that it shouldn't be fixed there (see below pull request).

Before recently, a simple work around would be in php.ini to set:

  register_argc_argv = On

However with recent changes, this seems to no longer work.

Original pull request: symfony#7354

See ticket symfony#7196

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

Commits
-------

2c6af95 [Process] fix issue where $_ENV contains array vals
@nicolas-grekas nicolas-grekas mentioned this issue Nov 22, 2021
Merged
nicolas-grekas added a commit that referenced this issue Nov 22, 2021
@nicolas-grekas
bug #44208 [Process] exclude argv/argc from possible default env vars…
26faa3a 
… (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[Process] exclude argv/argc from possible default env vars

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #44197
| License       | MIT
| Doc PR        | -

For some reason `getenv()` can contain these special keys.
Looks a lot like #7196

Commits
-------

36d254a [Process] exclude argv/argc from possible default env vars
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Process
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@beberlei @fabpot @mmucklo @oste @PaddyLock @heyflynn @robclancy
0