Latest Symfony Process make fastest screaming with PHP Warning: Array to string conversion about fastest HOT 19 CLOSED

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

from fastest.

@jan-j and others: could you please test against #177?

from fastest.

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.

from fastest.

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

from fastest.

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

from fastest.

Resolved in #177

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

from fastest.

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.

from fastest.

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 symfony/process@c209870 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'.

from fastest.

@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 😅

from fastest.

@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?

from fastest.

ping @francoispluchino. Please, could you validate this as you've introduced case conversion here #117?

from fastest.

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?

from fastest.

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;
}

from fastest.

@DonCallisto Regarding the commit symfony/process@e498803, 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?

from fastest.

@francoispluchino $argv is for sure an arrays. Our case conversions won't hit this control symfony/process@e498803#diff-a82f1f0b74256d93193ad7436778b39972f484e9286f70f203ef5b2aab052bc7R341 that before were filtered out by symfony/process@e498803#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 symfony/process@e498803#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?

from fastest.

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.

from fastest.

Thank you very much!

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

It work well and all the warning is gone 👍 .

from fastest.

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.

from fastest.

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

from fastest.