Check arguments in assertContainerBuilderHasServiceDefinitionWithMehodCall only if they were provided

[VasekPurchart]

Hello,

I was writing some compiler passes and noticed a little inconsistency - a lot of the assertContainerBuilderHas* methods have optional parameters, which is great, because you can choose the level of detail you want to check. This is especially needed if you are using a compiler pass to augment 3rd party code, where you don't have absolute control over it.

So for example assertContainerBuilderHasServiceDefinitionWithArgument takes argument value as a option and checks it only when given.

On the other hand assertContainerBuilderHasServiceDefinitionWithMehodCall has optional arguments parameter, but if you don't specify the argument, then the default is [] which means check that there are no arguments, which is totally different and could be confusing given the wording of the message (I got has a method call to "setLogger" with the given arguments..).

So I believe there is a need to differentiate no arguments given to check and check that there are no arguments, which I think can be translated as null vs [] and it would be understandable and even BC compatible, because if someone wants to check to empty array, they really should specify an empty array.

---

I think there may be other methods which accept arrays and behave similarly to the current state (assertContainerBuilderHasServiceDefinitionWithTag?), so If you will agree on this one with me and want me to, I can have a look at them too.

[matthiasnoback]

@stloyd What are your thoughts?

[stloyd]

@matthiasnoback I was looking at this before but was not sure how we could deal if someone wants to validate that call to some method was with exactly null values (setLogger(null) - check i.e. logger was not set or "removed" - edge case but still) & this is the only case I'm not sure about.

But in fact fix for given issue - calling with array as argument if no given - is correct, cause i.e. we may want to check that i.e. enableFeature() was called, while without this it would be enabledFeature([]) check.

[matthiasnoback]

@stloyd If I'm not mistaken, if you want to check for null as an argument you should provide [null], right?

Also, I didn't think it was possible to use null as a default value for an array-typed argument, but apparently it is? Hmm.

Anyway, I don't think we should spend too much time fixing this problem. I'd suggest keeping it open until maybe someone else stumbles on the inconsistency. Is that okay with you?

[VasekPurchart]

if you want to check for null as an argument you should provide [null]

Yea, I think so, we I can add this to the tests, if you want, to make it clearer and prevent breaking in the future?

Also, I didn't think it was possible to use null as a default value for an array-typed argument, but apparently it is? Hmm.

Yes, it is possible and used for this purpose usually, I think I have seen it for example in PHPUnit for exactly the same purpose.

Anyway, I don't think we should spend too much time fixing this problem. I'd suggest keeping it open until maybe someone else stumbles on the inconsistency. Is that okay with you?

If we don't see any problems with it and it in fact fixes an issue and inconsistency, I would rather propose to merge it and should a problem arise, it can probably be simply addressed by another issue? From my point of view the only thing there is left to decide is whether/which other methods this should apply to, which I would by happy to go trough.