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: https://github.com/zendframework/zend-db/blob/4c68f2c33beb76d63a59c5bd5e6c96c9763966d7/src/Sql/Expression.php#L91-L95

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!

[albertor24]

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

[michalbundyra]

Thanks, @albertor24!