RESOLVED in r46080, in svn://svn.zabbix.com/branches/dev/ZBX-8196

(1) The information about sortable columns cannot be taken from the API, since the sortable fields in the API may not correspond to the sortable column the in frontend. They must be specified for each page separately.

andrewtch RESOLVED, please see r46236.

jelisejev The changes to CApiWrapper need to be reverted as well.

andrewtch RESOLVED in r46344.

jelisejev CLOSED.

(2) The message must be displayed as a standard fatal error, that is, with a "ERROR: Zabbix has received an incorrect request." header and the error message as a detail. Also our messages start with "Cannot" so let's rephrase it as "Cannot sort by field %field%".

andrewtch RESOLVED in r46344.

jelisejev Since invalid_url() terminates code execution, the following code in not required:

// we do not want the profile to be updated with wrong value
return;

andrewtch RESOLVED in r46479. In fact, it is terminated in page_footer.php, but nevermind.

jelisejev CLOSED.

(3) Some coding style issues in validate_sort_and_sortorder():

1. The $allowedColumns needs to be type hinted;
2. We write "if ($allowedColumns)" instead of "if (count($allowedColumns) > 0)".

andrewtch RESOLVED in r46344.

jelisejev I've made a minor correction in r46473. Otherwise good, CLOSED.

(4) The validate_sort_and_sortorder() line in hostinventories.php is too long.

andrewtch RESOLVED in r46344.

jelisejev CLOSED.

(5) I've removed some unused code in r46341, please review.

andrewtch CLOSED. It's fine.

(6) While we're at it, let's add validation for the "sortorder" parameter as well.

andrewtch It is actually "validated":

if (!str_in_array($_REQUEST['sortorder'], array(ZBX_SORT_DOWN, ZBX_SORT_UP))) {
        $_REQUEST['sortorder'] = ZBX_SORT_UP;
}

you want to throw an error? (If yes, then do we need to use str_in_array or simple in_array then).

andrewtch RESOLVED in r46426.

jelisejev CLOSED.

(7) Strings added: 'Cannot sort by field "%1$s".', 'Incorrect sort direction "%1$s", must be either "%2$s" or "%3$s".'.

jelisejev CLOSED.

TESTED.

Closed via r46585.

(8) The values stored in the profiles table shouldn't be validated. The profiles table may contain outdated values from previous versions which will cause the frontend to crash.

<richlv> on the other hand, that would increase the possibility of messing things up. a more strict approach would be to validate them and remove/update outdated ones in dbpatches.

jelisejev I've reviewed similar places in 2.0 and 2.2 and they will be removed by dbpatches (9). But I'm not sure I haven't missed anything. Invalid values in the row are unlikely to cause serious problems, so I wouldn't want the whole page to crash because of it.

<richlv> can we show a warning but still display the page, like we do with other messages ?

andrewtch I suggest just ignoring wrong values pulled from the profile table. Dbpatches are also not needed, IMHO - these values will be overwritten automatically one the next sort, transparently for the user. Do you agree?

jelisejev I don't. We always assume that the data in the DB is valid. Therefore, we need to keep it that way. The profiles table is not an exception.

RESOLVED in r47104.

(9) We need db patches to remove the following values from the profiles table:

• web.latest.php.sort
• web.httpmon.php.sort
• web.httpconf.php.sort with value_str=h.hostid
• web.hostinventories.php.sort with value_str=hostid

andrewtch RESOLVED in r47136.

jelisejev

1. Each query must be performed in a separate patch. That way, if one of the queries fails for some reason, the server will be able to restart and continue the upgrade procedure from that specific patch.
2. You'll also need to bump the optional db version in schema.tmpl.

andrewtch RESOLVED in r47233. Please note that you missed the fact that I put #endif wrongly (which is also corrected now).

jelisejev Thanks, didn't notice that. CLOSED.

(10) Now request parameter validation doesn't work if the profiles table contains remembered values. Additionally, the validate_sort_and_sortorder() function shouldn't unset invalid values from the profiles table. The values in the profiles should be treated as valid. That's what DB patches are for.

And since we're at it, let's move the "sortfield" and "sortorder" validation out of the validate_sort_and_sortorder() function and into the main validation rules. Then validate_sort_and_sortorder() can be refactored into something more meaningful.

andrewtch RESOLVED in r47243. As discussed, validate_sort_and_sortoder() were not removed.

jelisejev CLOSED.

(11) The CProfile::has() function needs to be documented. And we use "=== null" instead of "is_null".

andrewtch RESOLVED, I removed it at all at r47250 since after (10) it is not needed anymore.

jelisejev CLOSED.

CLOSED, fixed in pre-2.3.3 r47393.