(1) note that simply opening that page does :

SELECT c.* FROM config c
SELECT c.* FROM config c

we probably should not select all fields, and probably should not do that twice...

oleg.egorov As was discussed, that is ok. We use select_config(), which return all fields

CLOSED

<richlv> why is it called twice here ? -> REOPENED

oleg.egorov I will try improve. But first select is in header and second in controller.
It the same as ZBX-8643 (5)
https://support.zabbix.com/browse/ZBX-8643?focusedCommentId=122395&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-122395

RESOLVED

sasha The same problem with Administration->Housekeeper page. Please fix it too. REOPENED.

Also it is called twice when saving changes on Administration->Others page.

oleg.egorov RESOLVED IN r51016

sasha CLOSED

(2) String changes

new strings:

• Field \"%1$s\" is not correct: %2$s

removed strings:

• "Display OK triggers" needs to be "0" or a positive integer.
• "Triggers blink on status change" needs to be "0" or a positive integer.
• Incorrect working time.
• Severity name cannot be empty.
• Zabbix server activity check interval

sasha RESOLVED

oleg.egorov CLOSED

FIXED IN svn://svn.zabbix.com/branches/dev/ZBX-9018 r50729 and r50731

(3) I made some improvements in r51109,51115. Please review.

oleg.egorov CLOSED

(4) Coding style:

1. In many places was removed "// VAR TYPE OPTIONAL FLAGS VALIDATION EXCEPTION", but in adm.housekeeper.php still exist
2. After copyright message only one line break in profiles.inc.php
3. Please add PHPDoc
4. Usually we don't use line break in this situation

   	else {
   		// set default configuration
   
   		$config = array();
   

sasha

1. 2. RESOLVED in r51313
3. I did not add new functions. In any case all changed functions will be moved into a server side API. I think it is unnecessary now.
4. if the comment is related to several blocks of code we need to add an empty line

sasha Updated PHP coding guidelines#Comments

oleg.egorov CLOSED

(5) Unnecessary default value for getRequest
For example in adm.workingtime.php:77

$config = select_config();

if (hasRequest('form_refresh')) {
	$data = array('work_period' => getRequest('work_period', $config['work_period']));
}
else {
	$data = array('work_period' => $config['work_period']);
}

Possible improve to:

if (hasRequest('form_refresh')) {
	$data = array('work_period' => getRequest('work_period'));
}
else {
	$config = select_config();
	$data = array('work_period' => $config['work_period']);
}

Default value for getRequest is never used. Because if exist $_REQUEST['form_refresh'], $_REQUEST['work_period'] exist too.

Same issue exist in other places.

sasha It will be fixed after implementation of the new validation of input parameters. These parameters should be "required" if form_refresh is set.

sasha Discussed with oleg.egorov. WON'T FIX

(6) validation.inc.php: 390
check_type validation

		if (!$timePeriodValidator->validate($var)) {
			$error = true;
			$message = _s('Field "%1$s" is not correct: ivalid time period.', $caption);
		}

There is possible use CTimePeriodValidator error message

I suggest use something like this:

		if (!$timePeriodValidator->validate($var)) {
			$error = true;
			$message = _s('Field "%1$s" is not correct: %2$s', $caption, $timePeriodValidator->getError());
		}

Because validator have more different error messages. Which can help to understand real problem.

sasha I agree. RESOLVED in r51314.

oleg.egorov CLOSED

(7) Audit log
For example Administration->General->Other
Changed "Refresh unsupported items (in sec)" fron 600 to 599, but in audit log:

Refresh unsupported items (in sec) "599".; Group for discovered hosts "Discovered hosts".; User group for database down messa...

1. Group for discovered hosts "Discovered hosts"... and other options I didn't change, but they in audit log description
2. In description is only new value. Previous value has been lost

Please change description, they should be as host/host groups... and other normal places

hosts.host: oleg => olegPC
hosts.name: oleg => OLEG PC
hosts.description: => Test

sasha I did not fix audit log problems. I leave it "as is". It will be fixed on server side API.

sasha As discussed with oleg.egorov. WON'T FIX

(8) If removed config
Now in 2.5 everywhere displayed error message

Unable to select configuration.

But now is OK, was loaded default values.

Open Administration->General
Change config value
After saving "Configuration updated" on the green background
Form refreshed with corrects values

But no row in DB

sasha I know about these issues. Let's discuss it later.

sasha RESOLVED in r51366.

oleg.egorov CLOSED

(9)

1. Open Trigger displaying options
2. Reset to default
3. Save
4. Uncheck blink from

• Unacknowledged PROBLEM events
• Acknowledged PROBLEM events
• Unacknowledged OK events

1. Save

• Unacknowledged PROBLEM events
• Acknowledged PROBLEM events
• Unacknowledged OK events

Have checked blink

sasha Thanks a lot! RESOLVED in r51315.

oleg.egorov CLOSED

TESTED

Fixed in pre-2.5.0 (trunk) r51380.