Fixes 381 - Allows zero as a parameter for the expression constructor

[albertor24]

Correctly sets Sql Expression Parameter when passing '0' as the second parameter in the constructor.

[michalbundyra]

@albertor24 What about 0 (as int)?

[albertor24]

I thought only '0' should be covered, as the argument signature is string | array. Would you prefer I handle both using is_numeric?

[michalbundyra]

@albertor24 I was looking on it and it looks like scalars should be allowed there as well. See:

zend-db/src/Sql/Expression.php

Lines 91 to 95 in 4c68f2c

	public function setParameters($parameters)
	{
	if (! is_scalar($parameters) && ! is_array($parameters)) {
	throw new Exception\InvalidArgumentException('Expression parameters must be a scalar or array.');
	}

It's quite old code (2012?) so it's possible that types in phpdocs are wrong...

[albertor24]

I updated the code to have the same behaviour as the setParameters function, without throwing to keep compatibility.

[michalbundyra]

@albertor24 I had a look on it again, and honestly I think the condition there should be as simple as:

if ($parameters !== null) {
    $this->setParameters($parameters);
}

So we don't really care about types in the constructor, as the setter is doing proper checks and throw exception if needed - and - we always pass the value through if provided (different than default null).
It would make the most sense for me. What do you think?

I would add also a test, that any other value provided (not a scalar and not an array) we should get exception on instantiation.

[albertor24]

That seems very reasonable. My only concern with that is that it could be considered a breaking change, since without those changes invalid values would have been ignored instead of raising an error.

Let me know if that's OK, and I will make all the required changes. Thank you very much for your help!

[michalbundyra]

@albertor24

invalid values would have been ignored instead of raising an error

What invalid values you have in mind? See this: https://3v4l.org/jVcag
so:

• false (bool),
• '' (empty string),
• '0' (zero as string),
• 0 (zero as int)
  all are scalars, so we are not gonna have an exception in the setter.

Also an empty array is not a problem - because please note - the default value of properties parameters is an empty array.

Do I miss something?

[albertor24]

Invalid values would be any objects. However, now that you point it out, I agree that most likely won't be used. I will make the required changes in a bit.

[michalbundyra]

@albertor24 Any object (even empty new \stdClass) will pass previous condition:

if (new \stdClass) {
  // .. this will be executed
}

so we are not changing functionality :) You can add the test - check if before you change and after, to confirm that is not changed.

[albertor24]

You're totally correct. Thank you very much!

[michalbundyra]

I think it will be better to use data provider.
Also you've missed empty string here.

[michalbundyra]

We can use ::class notation I believe.

[albertor24]

Fixed your comments. Thank you for spending the time to review the PR.

[michalbundyra]

@albertor24 LGTM 👍

[michalbundyra]

Thanks, @albertor24!