RESOLVED. Fixed in development branch svn://svn.zabbix.com/branches/dev/ZBX-8349 r46934. Please note that remaining validators were not covered and incomplete tests were not written as it was discussed. Please review changes caused by messageInvalid -> messageType in CCollectionValidator.

(1) String added: NONE

(2) I get a bunch of strict standards errors when running the CLdapAuthValidatorTest:

Strict standards: Declaration of CLdapAuthValidatorTest::testValidateValid() should be compatible with CValidatorTest::testValidateValid(array $params, $value) in /opt/lampp/htdocs/zabbix/trunk/frontends/php/tests/unit/include/classes/validators/CLdapAuthValidatorTest.php on line 63

Please enable all error reporting in bootstrap.php.

andrewtch RESOLVED by r47005.

jelisejev CLOSED.

(3) In CCollectionValidator:

I think it would be better to rewrite

// since second unique parameter can be absent, we call it in ugly way
$params = array($this->messageDuplicate, $duplicate[$this->uniqueField]);

if ($this->uniqueField2) {
	$params[] = $duplicate[$this->uniqueField2];
}

call_user_func_array(array($this, 'error'), $params);

as

if ($this->uniqueField2 === '') {
	$this->error($this->messageDuplicate, $this->uniqueField);
}
else {
	$this->error($this->messageDuplicate, $this->uniqueField, $this->uniqueField2);
}

andrewtch RESOLVED by r47015.

jelisejev CLOSED.

(4) The following test case in CCoolectionValidatorTest seems to be unnecessary:

			// this is a regression test
			// todo: rewrite messageInvalid to messageType
			array(
				array('messageType' => 'Not an array'),
				array(
					'string',
					100
				)
			)

andrewtch RESOLVED by r47015. Also added some test cases checking if uniqueField2 is null.

jelisejev CLOSED.

(5) The CIdValidator should be rewritten so that it wouldn't extend the CStringValidator class. Otherwise it inherits too much unnecessary functions. Also, let's use a single error message for both regexp and range checks.

RESOLVED in r47092. Now it's only messageInvalid.

jelisejev I've made some changes in r47103, please review.

andrewtch CLOSED, fine.

(6) The "messageType" message should be added to uses of CSetValidator where it's required.

andrewtch RESOLVED by r47027.

jelisejev

1. I would suggest to use the same error message for "messageType" as for "messageInvalid", that is, just write the the value is incorrect. Otherwise, messages like "Incorrect type of calculation type ..." look weird and don't really offer any useful info.
2. In CHttpTest::checkSteps() and CUser::checkInput() the "messageInvalid" message is set dynamically and references a specific object. "MessageType" must do the same.

andrewtch RESOLVED (in a bit of ugly way) in r47161.

jelisejev CLOSED.

(7) Add/complete CDecimalValidator class

andrewtch RESOLVED in r47087. I have gathered all my courage and rewritten all the CDecimalValidator. Please also verify messageFormat -> messageType transitions.

jelisejev I've made a minor improvement in r47107.

andrewtch Thanks, fine. Should we also accept values with leading "+" sign, like +123? Otherwise, CLOSED.

jelisejev Nah, "+" sign strings will not be supported.

(8) I've changed the CSetValidator to treat true, false and decimals as invalid values. Please review my changes in r47106.

andrewtch And what's the point? So it's not possible to find out if the value belongs to set of float values or is just "true", "false"? REOPENED.

jelisejev Because the current implementation doesn't support boolean and decimal values.

andrewtch So let's rename it to CLimitedSetValidator then.

jelisejev Agreed. Please do that.

andrewtch RESOLVED in r47153.

jelisejev CLOSED.

(9) Another issue with the CDecimalValidator.

		if (!is_numeric($value) || !preg_match('/^-?\d+(\.\d+)?$/', $value)) {
			$this->error($this->messageType, $value);

			return false;
		}

If we pass an array to this code, it will try to insert the array into a string, which is incorrect. We could solve this problem similarly to the CIdValidator: if a non-string and non-numeric value is passed, just trigger the "messageType" error without passing the value. But if it is a string or a number that doesn't match the regexp - trigger a "messageInvalid" error with the value.

andrewtch I did it in a more elegant way in r47167 as introducing additional message would require a lot of time and refactoring / testing throughout a code. Please check; RESOLVED.

jelisejev Good idea, I suggest to improve it: let's convert values to a matching string representation: arrays to "array", true to "true" etc. Something like:

protected function getPrintableValue($value) {
	if (is_array($value)) {
		return 'array';
	}
	elseif ($value === true) {
		return 'true';
	}
	elseif ($value === false) {
		return 'false';
	}
	elseif ($value === null) {
		return 'null';
	}

	return $value;
}

Then we can remove "messageType" at all and just use "messageInvalid" for both type of errors instead. Note, that this needs to be done in all validators, not just this one.

andrewtch RESOLVED in base CValidator r47260. I've also changed one test case to validate new behavior.

jelisejev

1. Please implement it in all validators and also refactor the "messageType" message.
2. In CValidator::stringify() gettype() will return "NULL" for a null value, should be "null" to be consistent with other types.
3. The opening curly brace must be placed on the same line as the function declaration.

andrewtch RESOLVED in r47444.

jelisejev CLOSED.

TESTED.

CLOSED. Fixed in pre-2.3.3 r47528.