Fix typehinted arguments not marking arguments as defined

[Taluu]

Fixes #991

Basically, array additions do some stuff that are not really expected ; if null is giving as a context constructor, it is skipped because it is how array additions behave.

Replacing it with a good old array_merge fixes that.

I'll add some unit tests for that.

[stof]

+ is the equivalent of array_replace, not array_merge. array_merge re-indexes the numeric keys, which looks wrong to me here

[Taluu]

Hum, indeed, but with array_replace, the null is inserted at the wrong place too (an argument is still squashed). So instead, I think we should append all args.

But with array_merge, it works, the arguments are appended. I'm not quite sure if the numerical indexes are really important though, as it is reorganized through the call to reorderArguments anyway ?

[Taluu]

The fix is indeed wrong, the array_merge could pose problems in the future (e.g using an unordered list of arguments or something like that).

Another solution (while leaving the + alone) would be to rewrite the prepare* methods so that the indexes are actually correctly matching the index of the linked parameter.

Actually, in my context, I have the following ; __construct(Foo $foo, Bar $bar, ?Baz $baz), and I'm giving the following services for my context :

default:
   suites:
     main:
       ...
       services: "MyContainer"
       contexts:
         - MyContext:
           - "@foo"
           - "@bar"
           - "@baz"

with @baz returning null on some conditions (optionnal dependency not met). I didn't actually try - ~ but I guess it should be the same thing.

In the getParameterNumberWithTypehintingValue method, I guess the fix would be to recognize null if the typehint of the parameter is nullable ?Baz, or at least accept a null value. WDYT ?

[Taluu]

@stof I Think I found the solution, which isn't replacing array additions with array_merge or array_replace. Basically, as the typehinted args were added without any treatments (like the other cases had), none were marked "defined". Adding a pass for that mark them as "defined", and thus the right index is used for the numberedArg pass (and probably the defaultArg pass too).

<?php
// .. constructor of the context (note the `?Baz`, but I think 
// it should work for `string`, no typehints, ... etc)
function __construct(Foo $foo, ?Baz $baz)
{
// ...
}

// in the mixed arg organizer
$typehinted = [0 => $foo];

// in the prepareNumberedArgument pass
return [0 => null]; // instead of expected [1 => null]

Thus, in the organizeArguments method, we would try to replace (without overwriting) [0 => $foo] with [0 => null]... The null is then discarded in favor of $foo. With this patch, it matches the right untreated parameter, making it trying to replace [0 => $foo] with [1 => null], hence additionning both arrays into [0 => $foo, 1 => null].

I'll add some tests for that.

[stof]

Can you add a scenario covering this ?

If it requires PHP 7.1+ nullable typehints, tag it as @php7.1 (btw, this made me saw that our CI setup dealing with these tags is broken for PHP 7+. It was not ready for changing the PHP major version)

[Taluu]

Actually this isn't using 7.1 or 7 actually. But I think that a fix for 7.1 should be done for nullable anyway, but I thought that this wasn't the scope.

I'll add the tests (features and unit)

[stof]

Well, if the bug can be triggered on all PHP versions, then writing a scenario covering it is even easier

[ciaranmcnulty]

@stof we could switch to https://github.com/ciaranmcnulty/version-based-test-skipper if needed

[Taluu]

Scenario added. I added it in the helper_container.feature, even though I am not sure it concerns only helper containers...

And I also added a note for the feature, as the first parameter isn't properly detected as a typehinted args, thus the last "numbered arg" would be a match and thus not deleted, while if there is a typehinted argument, it failed as expected (just undo the fix I did and it will fail as expected). cf #992 and #990

Do I also need to add a unit test ?

[everzet]

@Taluu thanks for a fix. Especially thank you for a scenario and update of the CHANGELOG - that does make my life much easier!