r37233

(1) same applies to
tr_status.php?&form_refresh=1&groupid=XXX&hostid=XXX&fullscreen=0
events.php?triggerid=XXX
charts.php?&form_refresh=1&fullscreen=0&groupid=XXX&hostid=XXX&graphid=XXX
overview.php?&form_refresh=1&groupid=XXX&application=XXX&type=0
hostinventoriesoverview.php?&form_refresh=1&groupid=XXX&groupby=
report2.php?filter_groupid=0&filter_hostid=XXX&triggerid=XXX

guntis.zarins RESOLVED in r37385.

jelisejev CLOSED.

(2) similar error srv_status.php?&form_refresh=1&fullscreen=0&period=XXX
incorrect period entered will produce error
Undefined variable: period_start [ in /frontends/php/srv_status.php:119]

guntis.zarins RESOLVED in r37385.

jelisejev CLOSED.

(3) there is issue when no parameters are added to the url, system should produce an error, that doenst happen
discoveryconf.php?form=update
overview.php?&form_refresh=1
actionconf.php?form=update
latest.php?&form_refresh=1
tr_status.php?&form_refresh=1
events.php? -possible its ok
charts.php?&form_refresh=1
screens.php?&form_refresh=1
maps.php?&form_refresh=1
hostinventoriesoverview.php?&form_refresh=1
hostinventories.php?&form_refresh=1
report2.php?

error message should state: Critical error. Field "XXX" is mandatory. example -screenedit.php

guntis.zarins discoveryconf.php and actionconf.php show new form, if id not given. Other pages showed previous selected choice. Not found any problem here.

iivs As discussed, this will not be fixed for now. The validation of ID's, however, is fixed.
CLOSED.

If in request is wrong groupid or hostid parameters ( have not permission or not exists ) PageFilter its values reset with default (equals 'All').

Is there required critical error message?

jelisejev In such cases the page filter should not be displayed at all.

RESOLVED in [svn://svn.zabbix.com/branches/dev/ZBX-6816] r37385.

(1) In charts.php:68:

• please format the parameters according to our guidelines;
• the nodeids and editable parameters are optional and can be ommited;
• it's best to pass the "output" => array("graphid") parameter to make sure that the API returns as little data as possible (currently output defaults to "refer" which is OK, but later we plan to change the default value to "extend" which will much more than required).

guntis.zarins RESOLVED in r37594.

jelisejev CLOSED.

(2) A couple of code improvements:

if (get_request('groupid', 0) > 0) {
	$groupId = available_groups($_REQUEST['groupid'], 0);
	if (!$groupId) {
		access_deny();
	}
}

• Instead of "get_request('groupid', 0) > 0" you can just write "get_request('groupid')";
• The available_groups() function is deprecated, it's better to use the groups.isreadable API method. The same for available_hosts() and available_triggers().

The same for other similar code blocks.

guntis.zarins
Probably can remove available_groups() and available_triggers() functions. Available_hosts() function is used for other purposes in hosts.php:586.
RESOLVED in r37594.

jelisejev You can replace available_hosts() in hosts.php with a corresponding API call and remove all of these functions.

guntis.zarins RESOLVED in r37738.

jelisejev CLOSED.

(3) The following parameters are no longer used and must be removed:

• latest.php: applicationid, close and open;
• auditlogs.php: groupid and hostid;
• disc_prototypes.php: groupid;
• services.php: group_serviceid.

guntis.zarins RESOLVED in r37637.

jelisejev CLOSED.

(4) In report2.php the following parameters must also be validated:

• hostgroupid
• tpl_triggerid
• filter_groupid
• filter_hostid

The validation must be different depending on the selected report mode.

guntis.zarins RESOLVED in r37637.

jelisejev
1. Please separate the validation rules for each report mode.
2. $availabilityReportMode is not a boolean variable, so instead of writing "if ($availabilityReportMode)" we should write "if ($availabilityReportMode) == AVAILABILITY_REPORT_BY_TEMPLATE"
3. The "itemdescription" field is only used to display the name of the item, it's value is not used after submitting the form, so it can be removed the validation rule. Also, it would be better to rename it to "item_name" since it contains the name of the item.

guntis.zarins RESOLVED in r37740

jelisejev CLOSED.

(5) In chart.php:

• the permission check on line 48 is not required;
• the filter parameter must not be overriden on line 56: the API should only check normal items;
• the screenid parameter seems to be unused.

guntis.zarins RESOLVED in r37637.

jelisejev I've made some minor corrections in r37714, please review.

guntis.zarins CLOSED.

(6) In chart2.php:

• the permission check on line 48 is not required;
• the screenid parameter seems to be unused.

guntis.zarins RESOLVED in r37637.

jelisejev CLOSED.

(7) In chart3.php the httptestid parameter must be validated.

guntis.zarins RESOLVED in r37637.

jelisejev It's better to move the permission check inside the "if ($httptestid = get_request('httptestid', false))" clause.

Also, the item validation on line 103 should only allow normal items, not prototypes and LLD rules.

guntis.zarins RESOLVED in r37742.

jelisejev CLOSED.

(8) In chart6.php - same as in (6).

guntis.zarins RESOLVED in r37637.

jelisejev CLOSED.

(9) In chart7.php - the "filter" parameter must not be overriden on line 56.

guntis.zarins RESOLVED in r37637.

jelisejev CLOSED.

(10) In chart_bar.php the following parameters must also be validated:

• hostids
• groupids
• items

The validation must be different depending on the selected report mode.

guntis.zarins RESOLVED in r37637.

jelisejev The validation must allow to use web items in the report. Also spaces are missing after commas.

guntis.zarins RESOLVED in r37742.

jelisejev CLOSED.

(11) In nodes.php the "nodeid" parameter must be validated.

guntis.zarins RESOLVED in r37637.

jelisejev CLOSED.

(12) In popup.php we need to validate:

• nodeid
• groupid
• hostid
• parent_discoveryid
• only_hostid

guntis.zarins RESOLVED in r37637.

jelisejev CLOSED.

(13) In report4.php the "media_type" parameter must be validated.

guntis.zarins RESOLVED in r37637.

jelisejev CLOSED.

(14) In report6.php the following parameters need to be validated:

• groupid
• hostids
• groupids

The validation must be different depending on the selected report mode.

guntis.zarins RESOLVED in r37653.

jelisejev
1. Empty hostids and groupids shouldn't cause a critical error, just a warning.
2. The validation must allow to use web items in the report.

guntis.zarins
I get critical error, if hostid and groupid is empty.
RESOLVED in r37742.

jelisejev That's the point, the should cause a warning, not a critical error. I've also made some changes in r37822. Please review.

iivs Unrelated to those last changes, but to r37742:
Removing items from reports bar, results in error: "No permissions to referred object or it does not exist!" and in addition

Undefined index: items [ in C:\Development\ZBX-6816\frontends\php\chart_bar.php:66]

Previously if no items were specified, graph was shown as empty. I think there should be no graph displayed when no items are specified (and after deleted).
This minor issue RESOLVED in r37909
Empty hostids and groupids now show Warning: RESOLVED in r37920

jelisejev Great! There's one more problem left, when submitting the "compare values for multiple periods" form without an item, a warning must be shown instead of a critical error.

iivs Changed critical to warning.
RESOLVED in r37980

jelisejev Minor corrections in r37987. Please review.

iivs REVIEWED.
Thanks!
CLOSED.

oleg.egorov Undefined indexes and errors after item removing from "Distribution of values for multiple periods" and "Distribution of values for multiple items" tabs

REOPEN

iivs RESOLVED in r38411

oleg.egorov CLOSED

(15) In screens.php we need to validate:

• groupid
• hostid

If an unexisting elementid is given, it should display the standard access deny message instead of the one displayed currently.

The following parameters need to be removed:

• tr_groupid
• tr_hostid

guntis.zarins RESOLVED in r37637.

jelisejev
1. When calling screen.get it's better to just request the ID of the screen, not the number if results. And it's better to rename $screenItems to just $screens because screen.get will return an array of screens, not screen items.
2. The code responsible for displaying the old "ERROR: Screen with ID "162" does not exist." must be removed.

guntis.zarins RESOLVED in r37742.

jelisejev I've removed some more unused code in r37823. Please review.

iivs REVIEWED.
CLOSED.

(16) In slides.php we need to validate:

• groupid
• hostid
• elementid

The following parameters need to be removed:

• tr_groupid
• tr_hostid

guntis.zarins RESOLVED in r37637.

jelisejev $slides is an incorrect name for this variable, since it contains a slideshow, not slides.

guntis.zarins RESOLVED in r37742.

jelisejev CLOSED.

(17) In tr_logform.php we need to validated:

• itemid
• triggerid

The following parameters need to be removed:

• groupid
• hostid

guntis.zarins RESOLVED in r37637.

jelisejev Creating a trigger requires write permissions to the item.

guntis.zarins RESOLVED in r37743.

jelisejev CLOSED.

(18) In users.php we need to validate "filter_usrgrpid".

guntis.zarins RESOLVED in r37637.

jelisejev userid and filter_usrgrpid validation must require write permissions.

guntis.zarins RESOLVED in r37743

jelisejev CLOSED.

(19) There's a duplicate permission check in hostinventories.php:80. It should be removed.

guntis.zarins RESOLVED in r37743

jelisejev CLOSED.

(20) There are a lot of minor coding style issues like missing spaces. Please review your changes and correct them.

guntis.zarins RESOLVED in r37743

jelisejev I've made some additional changes in r37821 and r37826, please review.

Eduards REOPEN, in getOnlyHostParam() "param" variable is redundant

iivs RESOLVED in r37922, r37924 (code refactoring), r37925 (merged newest changes from trunk)

jelisejev CLOSED.

TESTED.

Please review (14) before merging.

Fixed in pre-2.1.3 (trunk) r37990

issues found on hudson, please see comments

(21) Use of two links with incorrect/correct itemid for following links related to Reports-Bar reports

http://hudson/~hudson/trunk-FRONTEND-POSTGRESQL/frontends/php/report6.php?items[0][caption]=Agent+ping&items[0][itemid]=23455&items[0][color]=009900&items[0][calc_fnc]=2&items[0][axisside]=0&report_show=Show

http://hudson/~hudson/trunk-FRONTEND-POSTGRESQL/frontends/php/report6.php?items[0][caption]=Agent+ping&items[0][itemid]=11111&items[0][color]=009900&items[0][calc_fnc]=2&items[0][axisside]=0&report_show=Show

Please see TEST_SERVER_NAME Bar reports - Google Chrome_087.png for more information

iivs RESOLVED in svn://svn.zabbix.com/branches/dev/ZBX-6816 r38287

oleg.egorov CLOSED

(22) Use of two links with incorrect/correct params for following links related to Reports-Bar reports

http://hudson/~hudson/trunk-FRONTEND-POSTGRESQL/frontends/php/report6.php?items[0][caption]=Agent+ping&items[0][itemid]=abc&items[0][color]=009900&items[0][calc_fnc]=2&items[0][axisside]=0&report_show=Show

http://hudson/~hudson/trunk-FRONTEND-POSTGRESQL/frontends/php/report6.php?items[0][caption]=Agent+ping&items[0][itemid]=&items[0][color]=009900&items[0][calc_fnc]=2&items[0][axisside]=0&report_show=Show

Please see TEST_SERVER_NAME Bar reports - Google Chrome_088.png for more information

iivs RESOLVED in svn://svn.zabbix.com/branches/dev/ZBX-6816 r38287

oleg.egorov CLOSED

(23) Please add in check_fields mandatory field "periods" (Distribution of values for multiple items tab)
Disallow display bar report without period data

iivs RESOLVED in r38455, r38456 (code refactoring)

oleg.egorov CLOSED

(24) Undefined index: calc_fnc [report6.php:208 → bar_report_form() in C:\xampp\htdocs\ZBX-6816\frontends\php\include\reports.inc.php:98]Undefined index: axisside [report6.php:208 → bar_report_form() in C:\xampp\htdocs\ZBX-6816\frontends\php\include\reports.inc.php:99]

If in URL don't exist item parameters

iivs RESOLVED in r38455, r38456 (code refactoring)

oleg.egorov CLOSED

(25) http://localhost/ZBX-6816/frontends/php/report6.php?sid=f65b307bb679dfd3&form_refresh=1&form=1&config=1&report_timesince=20130912093609&report_timetill=20130913093609&items[23630][itemid]=23630&items[23630][caption]=test&items[23630][color]=009900&items[23630][calc_fnc]=2&items[23630][axisside]=0&title=Report+1&xlabel=&ylabel=&scaletype=3&report_timesince_day=12&report_timesince_month=09&report_timesince_year=2013&report_timesince_hour=09&report_timesince_minute=36&report_timetill_day=13&report_timetill_month=09&report_timetill_year=2013&report_timetill_hour=09&report_timetill_minute=36&report_show=Show

if items is array

array_flip(): Can only flip STRING and INTEGER values! [report6.php:105 → CAPIObject->get() → CAPIObject->__call() → czbxrpc::call() → czbxrpc::callAPI() → call_user_func() → CItem->get() → dbConditionInt() → array_flip() in C:\xampp\htdocs\ZBX-6816\frontends\php\include\db.inc.php:1058]
Error in query [SELECT COUNT(DISTINCT i.itemid) AS rowscount FROM items i WHERE i.flags IN (0,4) AND ] [You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1]
mysqli_fetch_assoc() expects parameter 1 to be mysqli_result, boolean given [report6.php:105 → CAPIObject->get() → CAPIObject->__call() → czbxrpc::call() → czbxrpc::callAPI() → call_user_func() → CItem->get() → DBfetch() → mysqli_fetch_assoc() in C:\xampp\htdocs\ZBX-6816\frontends\php\include\db.inc.php:616]
mysqli_free_result() expects parameter 1 to be mysqli_result, boolean given [report6.php:105 → CAPIObject->get() → CAPIObject->__call() → czbxrpc::call() → czbxrpc::callAPI() → call_user_func() → CItem->get() → DBfetch() → mysqli_free_result() in C:\xampp\htdocs\ZBX-6816\frontends\php\include\db.inc.php:618]

if color is array

preg_match() expects parameter 2 to be string, array given [report6.php:186 → validateBarReportItems() → CStringValidator->validate() → preg_match() in C:\xampp\htdocs\ZBX-6816\frontends\php\include\classes\validators\string\CStringValidator.php:96]
Array to string conversion [report6.php:186 → validateBarReportItems() → CStringValidator->validate() → CValidator->error() → vsprintf() in C:\xampp\htdocs\ZBX-6816\frontends\php\include\classes\validators\CValidator.php:111]

if calc_fnc, axisside... is array

Illegal offset type in isset or empty [report6.php:186 → validateBarReportItems() → CSetValidator->validate() in C:\xampp\htdocs\ZBX-6816\frontends\php\include\classes\validators\CSetValidator.php:49]

if groupid is array

array_flip(): Can only flip STRING and INTEGER values! [report6.php:81 → CAPIObject->isReadable() → CAPIObject->__call() → czbxrpc::call() → czbxrpc::callAPI() → call_user_func() → CHostGroup->isReadable() → CHostGroup->get() → dbConditionInt() → array_flip() in C:\xampp\htdocs\ZBX-6816\frontends\php\include\db.inc.php:1058]
Error in query [SELECT COUNT(DISTINCT g.groupid) AS rowscount FROM groups g WHERE ] [You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1]
mysqli_fetch_assoc() expects parameter 1 to be mysqli_result, boolean given [report6.php:81 → CAPIObject->isReadable() → CAPIObject->__call() → czbxrpc::call() → czbxrpc::callAPI() → call_user_func() → CHostGroup->isReadable() → CHostGroup->get() → DBfetch() → mysqli_fetch_assoc() in C:\xampp\htdocs\ZBX-6816\frontends\php\include\db.inc.php:616]
mysqli_free_result() expects parameter 1 to be mysqli_result, boolean given [report6.php:81 → CAPIObject->isReadable() → CAPIObject->__call() → czbxrpc::call() → czbxrpc::callAPI() → call_user_func() → CHostGroup->isReadable() → CHostGroup->get() → DBfetch() → mysqli_free_result() in C:\xampp\htdocs\ZBX-6816\frontends\php\include\db.inc.php:618]

...

Same problems exist in popup

Will be fixed in ZBX-6996

CLOSED

(26) In item popup field parameter
In first item select we have "host:item"
In second only "item"
But if exist caption, then parameter = caption

Same problem in "Compare values for multiple periods"

iivs RESOLVED in r38495

oleg.egorov popup_bitem.php:117-120

I think better change to:

$host = get_request('host');
$itemName = get_request('name');
if ($host && $itemName) {
	$caption = $host['name'].NAME_DELIMITER.$itemName;
}

iivs RESOLVED in r38514

oleg.egorov CLOSED

(27) If groupids != array

Argument 1 passed to CHostGroup::isReadable() must be of the type array, string given [report6.php:81 → CAPIObject->isReadable() → CAPIObject->__call() → czbxrpc::call() → czbxrpc::callAPI() → call_user_func() → CHostGroup->isReadable() in C:\xampp\htdocs\ZBX-6816\frontends\php\api\classes\CHostGroup.php:1005]

Same problem if hostids != array

Argument 1 passed to CHost::isReadable() must be of the type array, string given [report6.php:84 → CAPIObject->isReadable() → CAPIObject->__call() → czbxrpc::call() → czbxrpc::callAPI() → call_user_func() → CHost->isReadable() in C:\xampp\htdocs\ZBX-6816\frontends\php\api\classes\CHost.php:1609]

Will be fixed in ZBX-6996

CLOSED

(28) Coding style

Please rename functions: barReportForm, barReportForm2, barReportForm3

Review my changes in r38486

iivs REVIEWED. Additional code refactoring see in r38488
function names RESOLVED in r38489

oleg.egorov CLOSED

(29) Distribution of values for multiple periods report tab
Add item
Function: Count

Add -> ERROR: Missing "calc_fnc" field for item.

Same problem in other tabs

iivs RESOLVED in r38515

oleg.egorov CLOSED

(30) http://localhost/ZBX-6816/frontends/php/report6.php?sid=1546c7e5aa444e2a&form_refresh=1&form=1&config=1&report_timesince=20130915152424&report_timetill=20130916152424&title=Report+1&xlabel=&ylabel=&scaletype=3&report_timesince_day=15&report_timesince_month=09&report_timesince_year=2013&report_timesince_hour=15&report_timesince_minute=24&report_timetill_day=16&report_timetill_month=09&report_timetill_year=2013&report_timetill_hour=15&report_timetill_minute=24&new_graph_item[caption]=aaaa_item_2&new_graph_item[itemid]=|23687|&new_graph_item[color]=009900&new_graph_item[calc_fnc]=2&new_graph_item[axisside]=0

new_graph_item[itemid]=|23687|

Undefined index: caption [report6.php:221 → valueDistributionFormForMultiplePeriods() in C:\xampp\htdocs\ZBX-6816\frontends\php\include\reports.inc.php:84]
Undefined index: host [report6.php:221 → valueDistributionFormForMultiplePeriods() in C:\xampp\htdocs\ZBX-6816\frontends\php\include\reports.inc.php:94]
Undefined index: name [report6.php:221 → valueDistributionFormForMultiplePeriods() → itemName() in C:\xampp\htdocs\ZBX-6816\frontends\php\include\items.inc.php:642]

iivs RESOLVED in r38535

oleg.egorov Please fix coding style errors, camelCase for $already_exist, $new_gitem
And please rename $data to $item

REOPEN

iivs RESOLVED in r38554

oleg.egorov CLOSED

Tested, by before merge please fix and close (30)

Fixed in pre-2.1.5 (trunk) r38557