StaticValidator test is incorrect (false positive)

[alexdenvir]

Opening an issue as a result of findings in my pull request #15. Here the issue has been removed, but not explicitly addressed.

There is a test in StaticValidatorTest (and ValidatorChainTest) which test the StaticValidator and Between:

    public function testExecuteValidWithParameters()
    {
        $this->assertTrue(StaticValidator::execute(5, 'Between', [1, 10]));
        $this->assertTrue(StaticValidator::execute(5, 'Between', ['min' => 1, 'max' => 10]));
    }

The first assertion implies (or at least creates some confusion, as evidenced in the pull request) that you can pass an array of options in, and they are passed to the actual validator as individual parameters. This is definitely not the case.
In this example, the Between validator will end up with an option called '0', with a value of '1', and an option called '1', with a value of '10'. The test currently passes, because the Between validator has a default value for min and max (0 and PHP_INT_MAX respectively).

To show more detail:

 % php -a
Interactive mode enabled

php > require_once "vendor/autoload.php";
php > var_dump(\Zend\Validator\StaticValidator::execute(5, 'Between', ['min' => 1, 'max' => 10]));
bool(true)
php > var_dump(\Zend\Validator\StaticValidator::execute(0, 'Between', ['min' => 1, 'max' => 10]));
bool(false)
php > var_dump(\Zend\Validator\StaticValidator::execute(100, 'Between', ['min' => 1, 'max' => 10]));
bool(false)
php > var_dump(\Zend\Validator\StaticValidator::execute(5, 'Between', [1, 10]));
bool(true)
php > var_dump(\Zend\Validator\StaticValidator::execute(0, 'Between', [1, 10]));
bool(true)
php > var_dump(\Zend\Validator\StaticValidator::execute(100, 'Between', [1, 10]));
bool(true)
php > $validator = new \Zend\Validator\Between([1, 10]);
php > var_dump($validator);
class Zend\Validator\Between#2 (5) {
  protected $messageTemplates =>
  array(2) {
    'notBetween' =>
    string(57) "The input is not between '%min%' and '%max%', inclusively"
    'notBetweenStrict' =>
    string(53) "The input is not strictly between '%min%' and '%max%'"
  }
  protected $messageVariables =>
  array(2) {
    'min' =>
    array(1) {
      'options' =>
      string(3) "min"
    }
    'max' =>
    array(1) {
      'options' =>
      string(3) "max"
    }
  }
  protected $options =>
  array(5) {
    'inclusive' =>
    bool(true)
    'min' =>
    int(0)
    'max' =>
    int(9223372036854775807)
    [0] =>
    int(1)
    [1] =>
    int(10)
  }
  protected $value =>
  NULL
  protected $abstractOptions =>
  array(7) {
    'messages' =>
    array(0) {
    }
    'messageTemplates' =>
    array(2) {
      'notBetween' =>
      string(57) "The input is not between '%min%' and '%max%', inclusively"
      'notBetweenStrict' =>
      string(53) "The input is not strictly between '%min%' and '%max%'"
    }
    'messageVariables' =>
    array(2) {
      'min' =>
      array(1) {
        ...
      }
      'max' =>
      array(1) {
        ...
      }
    }
    'translator' =>
    NULL
    'translatorTextDomain' =>
    NULL
    'translatorEnabled' =>
    bool(true)
    'valueObscured' =>
    bool(false)
  }
}

I should point out that in my pull request, I have removed this assertion from the tests, as the Between validator no longer has default values.

[weierophinney]

Question, @alexdenvir : Does #15 resolve the issue? or does further diligence need to be done in the StaticValidator tests?

[alexdenvir]

@weierophinney - Yes, #15 resolved the issue, although it was only one way to resolve it.

As explained in that PR - the Between validator currently requires options to be passed through the constructor, but it only checks that 2 options have been set (because the rest of the if statement currently always evaluates to true).
As shown in the example above, you can pass an array of options to the Between validator without array keys - which means that there are 2 options, but not associated with the min and max options:

protected $options =>
  array(5) {
    'inclusive' =>
    bool(true)
    'min' =>
    int(0)
    'max' =>
    int(9223372036854775807)
    [0] =>
    int(1)
    [1] =>
    int(10)
  }

Because Between also has default options (which were removed in #15), the validator still works, but not as expected, thus the false positive on this test.

My PR resolved this by removing the default options, and throwing an exception in the isValid method rather than the constructor if the validator had no min and max options set - which is in fact suggested by the docblock of isValid in ValidatorInterface (assuming that it is impossible to validate $value when no min and max are set) :

    /**
     * Returns true if and only if $value meets the validation requirements
     *
     * If $value fails validation, then this method returns false, and
     * getMessages() will return an array of messages that explain why the
     * validation failed.
     *
     * @param  mixed $value
     * @return bool
     * @throws Exception\RuntimeException If validation of $value is impossible
     */
    public function isValid($value);

Another option is to rework the Between constructor and remove the default options:

    /**
     * Options for the between validator
     *
     * @var array
     */
    protected $options = array(
        'inclusive' => true,  // Whether to do inclusive comparisons, allowing equivalence to min and/or max
        'min'       => null,
        'max'       => null,
    );

    /**
     * Sets validator options
     * Accepts the following option keys:
     *   'min' => scalar, minimum border
     *   'max' => scalar, maximum border
     *   'inclusive' => boolean, inclusive border values
     *
     * @param  array|Traversable $options
     *
     * @throws Exception\InvalidArgumentException
     */
    public function __construct($options = null)
    {
        if ($options instanceof Traversable) {
            $options = ArrayUtils::iteratorToArray($options);
        }
        if (!is_array($options)) {
            $options = func_get_args();
            $temp['min'] = array_shift($options);
            if (!empty($options)) {
                $temp['max'] = array_shift($options);
            }

            if (!empty($options)) {
                $temp['inclusive'] = array_shift($options);
            }

            $options = $temp;
        }

        parent::__construct($options);

        if ($this->getMin() === null || $this->getMax() === null) {
            throw new Exception\InvalidArgumentException("Missing option. 'min' and 'max' have to be given");
        }
    }

Alternatively, StaticValidator could be updated to check if the options passed to it is an associative array, and if not pass each array value as a separate scalar value.
If I'm understanding the argument unpacking feature of PHP 5.6 correctly, something like this would work without changing the tests:

    /**
     * @param  mixed    $value
     * @param  string   $classBaseName
     * @param  array    $args          OPTIONAL
     * @return bool
     */
    public static function execute($value, $classBaseName, array $args = array())
    public static function execute($value, $classBaseName, array $args = array())
    {
        $plugins = static::getPluginManager();

        if (array_keys($args) !== range(0, count($args) - 1)) {
            $validator = $plugins->get($classBaseName, ...$args);
        } else {
            $validator = $plugins->get($classBaseName, $args);
        }

        return $validator->isValid($value);
    }

Obviously I'm not saying the minimum version of php to 5.6 (at least yet 😛). I am happy to throw together a new pull request to address this with whatever option I've given or alternatives suggested by others is preferred

[alexdenvir]

Forgot to mention, obviously all the suggestions I've made require changes to tests as well

[weierophinney]

@alexdenvir We can use ArrayUtils::isAssocArray() to determine if we have an assoc array, and, if not, we don't pass the arguments to $plugins->get(). We can't use variadic for two reasons:

• Requires PHP 5.6 (which you already noted)
• AbstractPluginManager::get() only expects 2 arguments, so passing the $args as variadic arguments is a moot point anyways.

The main thing right now is that the tests are simply written incorrectly, and need to be updated to illustrate correct usage. We can add a few more test cases as well to validate that they are passing the options correctly (e.g., use -5 -> -1 as the range for the Between validator). I'm noting that on my todo.

[alexdenvir]

@weierophinney Yeah, the argument unpacking suggestion was put together purely to show an alternative (which would need more work than what's shown here). I honestly didn't think of using ArrayUtils to check if an array is associative or not, makes sense really (although I cant see isAssocArray, did you mean hasNumericKeys / hasIntergerKeys / !hasStringKeys?).

I'm going to throw together another pull request that will fix up the tests.

I still think the Between validator needs to be fixed as well though - perhaps an alternate version of the reworked constructor/no defaults suggestion I made above - at least until stateless validators become a thing:

public function __construct($options = null)
    {
        if ($options instanceof Traversable) {
            $options = ArrayUtils::iteratorToArray($options);
        }

        if (!is_array($options)) {
            $options = func_get_args();
        }

        if (ArrayUtils::hasNumericKeys($options)) {
            $temp['min'] = array_shift($options);
            if (!empty($options)) {
                $temp['max'] = array_shift($options);
            }

            if (!empty($options)) {
                $temp['inclusive'] = array_shift($options);
            }

            $options = $temp;
        }

        parent::__construct($options);

        if ($this->getMin() === null || $this->getMax() === null) {
            throw new Exception\InvalidArgumentException("Missing option. 'min' and 'max' have to be given");
        }
    }

[Maks3w]

Forwarding my message posted on #26 for a single thread of discussion:

I see three cases here:

• Constructor uses func_get_args
• $args is used for constructor arguments so could be a bidimensional array passing the options as first argument
  `[ [ options ] ]
• $args is the array options accidentaly used as constructor arguments list

Because argument name is $args which means arguments list I think the current implementation is the correct. The intention of provide a constructor argument list.

Provide a constructor argument list allow to instantiate constructors with different constructor signatures and not only the ones based in AbstractValidator.

My opinion is won't fix.

[weierophinney]

As noted on #26, I disagree with all 3 assertions.

StaticValidator was ported from ZF1's Zend_Validate::is static method. That method does indeed allow discrete constructor arguments, and uses reflection in order to pass them.

However, we decided when refactoring for ZF2 to make this a static class. Additionally, we decided that all validators — indeed, anything that would be managed by a plugin manager, including filters, view helpers, controller plugins, and more — would allow passing an array of options to the constructor; this allowed us to create a simpler paradigm for plugins, as you can then pass in named arguments, instead of needing to know argument order. We refactored plugins to allow discrete arguments, with all arguments optional, and the first allowing overloading so that an associative array of options detailing all constructor options (and otherwise) could be passed in at once. Zend\ServiceManager\AbstractPluginManager was written such that it's get() implementation allows passing that array of options to the constructor, which allows us to mark all plugins we ship as invokable by default.

If your plugin does not follow that paradigm, and you want to be able to pass in options, you must write a factory, and the factory constructor must accept the creation options to its constructor; you can then decide what to do with them when the factory is invoked.

The points I'm making here are:

• The only use case ZF2 has attempted to support is passing an associative array of options to the constructor, and/or discrete optional arguments.
• The plugin manager will pass arguments passed in the second argument to get() as those options.
• $args in StaticValidator is passed as that second get() argument.

This is the only supported, documented use case. The tests were incorrect, and we were not validating that we had valid arguments passed to StaticValidator. The patch in #26 corrects this.

[alexdenvir]

I've had a read over of #26, and I'm happy putting my 👍 to it as issue reporter that it fixes the error with the test.

The Between validator still has its issue with accepting an array that does not set the minimum and maximum values if instantiated by directly calling new Between([5, 10]), but I guess that really is a separate issue.

Additionally, we decided that all validators — indeed, anything that would be managed by a plugin manager, including filters, view helpers, controller plugins, and more — would allow passing an array of options to the constructor; this allowed us to create a simpler paradigm for plugins, as you can then pass in named arguments, instead of needing to know argument order.

Having read this comment, does it make sense for Zend\ServiceManager\AbstractPluginManager to also check the array of options (if set) that it is an associative array? I appreciate that is a much bigger task than just correcting the behaviour of StaticValidator, but it might be something to consider for a future release?