Latest Symfony Process make fastest screaming with PHP Warning: Array to string conversion

[kocoten1992]

After this PR, https://github.com/symfony/process/commit/e498803a6e95ede78e9d5646ad32a2255c033a6a

Our example on readme always screaming when running tests:

# find tests/ -name "*Test.php" | ./vendor/liuggio/fastest/fastest "vendor/phpunit/phpunit/phpunit -c app {};"
PHP Warning:  Array to string conversion in .../vendor/symfony/process/Process.php on line 342

Think it on Symfony process but I'm not sure yet (or is it on us ?).

[DonCallisto]

Looking here https://github.com/liuggio/fastest/blob/master/src/Process/EnvCommandCreator.php#L30-L37, I highly suspect that's your code "fault" and you're putting something unexpected (array value) in your env. I would suggest to print out before any test the $env and check if all's fine. I'm closing it for the moment, feel free to reopen if you've verified issue is on fastest side.

[jan-j]

As far as I can tell this bug has started recently with the latest security update for symfony/process (version v4.4.35/v5.3.12). This https://github.com/symfony/process/commit/c2098705326addae6e6742151dfade47ac71da1b commit specifically.

The problem is the argv key from $_SERVER variable which gets passed as ARGV in $env variable into Process. Before the above commit all array values were filtered out, but now it skips based on 'argc' !== $k && 'argv' !== $k condition, which doesn't apply since $k = 'ARGV'.

[DonCallisto]

@jan-j Yeah, I've seen the commit suggested but I don't think that's related at all with fastest. Didn't understood if your comment was reinforcing my point or not tho :sweat_smile:

[jan-j]

The problem I see it is the compatibility between iluggio/fastest and latest version of symfony/process.

array_change_key_case([...], CASE_UPPER) uppercases argv key and because of that it is not filtered out in symfony/process and array value from $env['ARGV'] gets converted to string with the warning.

This doesn't seem like something that can be fixed by the library user. It has to be fixed in one of these packages, unless I'm missing something.

[DonCallisto]

@jan-j woah, I've totally missed the case conversion of env keys, thanks for pointing it out. Well, I think we should get rid of case conversion as it takes into account both super globals "$_SERVERand$_ENV` which we should not take care. We're setting our variables and we should take care only of those. If user isn't following a convention (e.g.: have superglobals keys all upper case) it's up to him to fix the things.

WDYT?

[DonCallisto]

ping @francoispluchino. Please, could you validate this as you've introduced case conversion here https://github.com/liuggio/fastest/pull/117?

[DonCallisto]

Ok, got it. Case conversion was there only for preventing "duplicates", not for any other reason. If we get rid of this, we could fall into scenarios with duplicates when same string has different case. Maybe it's legit? What would someone expect from merging two env variables with different case?

[jan-j]

I am really in no position to tell how it should work, but maybe it should be preserving the case of env variable keys, but using a case insensitive key comparison when merging. Something like this:

private function mergeEnvVariables(array ...$envVarsSources): array
{
    $mergedEnvVars = [];
    $lowerCaseKeysMerged = [];

    foreach ($envVarsSources as $envVarsSource) {
        foreach ($envVarsSource as $key => $value) {
            if (array_key_exists(strtolower($key), $lowerCaseKeysMerged)) {
                continue;
            }

            $mergedEnvVars[$key] = $value;
            $lowerCaseKeysMerged[strtolower($key)] = true;
        }
    }

    return $mergedEnvVars;
}

[DonCallisto]

Yes, I was thinking the same as should be the safest way to handle this case where mixed variables could come from the outside.

[francoispluchino]

@DonCallisto Regarding the commit symfony/process@e498803a6e95ede78e9d5646ad32a2255c033a6a, I'm not sure the problem is with the PR #117. However, this one is starting to date, and I can't remember at all why I added this action (I avoid changes like this, so there was a definite reason at the time). On this point of the PR, I agree with you, I think @jan-j improvement proposal is interesting and should replace the use of array_change_key_case.

Since the latest version of Symfony Process uses the format $ _ENV + $ env to merge 2 array, this should behave like a [union] (https://www.php.net/manual/en/language.operators.array.php) and there is therefore no longer any verification on the value that must be a string (and can be added in the env array).

The error message indicates an attempt to convert an array to a string, which indicates that a variable in one of the 2 arrays does not have a scalar value, but an array of values.

This is because Symfony Process creates the variable as a string in the format <key>=<value>. Given that variables that are not strings are now included, the error occurs.

@jan-j Can you check that there is not an environment variable value with an array?

[DonCallisto]

@francoispluchino $argv is for sure an arrays. Our case conversions won't hit this control https://github.com/symfony/process/commit/e498803a6e95ede78e9d5646ad32a2255c033a6a#diff-a82f1f0b74256d93193ad7436778b39972f484e9286f70f203ef5b2aab052bc7R341 that before were filtered out by https://github.com/symfony/process/commit/e498803a6e95ede78e9d5646ad32a2255c033a6a#diff-a82f1f0b74256d93193ad7436778b39972f484e9286f70f203ef5b2aab052bc7L1159-L1161.

$argv contains something like

"ARGV" => array:4 [
    0 => "bin/fastest"
    1 => "-vv"
    2 => "--process=7"
    3 => "bin/behat --stop-on-failure --config tests/behat/behat.yml {}"
  ]

[taken from a test of mine]

As $argv refers, as expected, to fastest itself, as it is run outside the symfony process, I highly suspect (even more after looking at https://github.com/symfony/process/commit/e498803a6e95ede78e9d5646ad32a2255c033a6a#diff-a82f1f0b74256d93193ad7436778b39972f484e9286f70f203ef5b2aab052bc7L1159-L1161) that we should not only leave case untouched, but also get rid of them as, like said before, they're be part of fastest and not of the test command spinned by fastest itself (it's already contained entirely, in my example, at position 3.

Any toughts?

[DonCallisto]

I'm working on this https://github.com/liuggio/fastest/tree/issue_176 Still missing merge part and tests.

[jan-j]

Hello again! It's been almost 2 weeks since the last update so I just wanted to check with you and offer some assist to finish the fix if you feel like that would be helpful.

[DonCallisto]

@jan-j and others: could you please test against https://github.com/liuggio/fastest/pull/177?

[kocoten1992]

Thank you very much!

I use composer require liuggio/fastest:dev-issue_176 --dev.

It work well and all the warning is gone :+1: .

[DonCallisto]

This weekend I'll merge this PR and make a new release. To all people involved in this conversation, plase, could you test the patch (or review the code) before the end of this week?

Thanks.

[jan-j]

I've tested it on our codebase and the patch works. Also for your information the issue was also addressed on the Symfony side https://github.com/symfony/symfony/issues/44534.

[DonCallisto]

@jan-j Thanks. I think it's safer to keep also this change in fastest as we should not interfere with superglobals that someone could depend on. Gonna merge it soon.

[DonCallisto]

Resolved in #177

Released new tagged version (1.9.0) -> https://github.com/liuggio/fastest/releases/tag/v1.9.0