Avatar

questions about comparisions in the PHP code base (Technics)

by Auge ⌂ @, Tuesday, November 07, 2017, 11:31 (104 days ago)
edited by Auge, Tuesday, November 07, 2017, 11:45

Hello

During refactoring the include-files I found several PHP-code blocks I tend to describe as at least disadvantageous, with a bad performance or not well maintainable.

I found bunches of strings to compare with the value of an URL-parameter (i.e. for sorting the users list), same for recurring test for sorting direction (ASC, DESC). I would like to replace such if-munsters with a simple in_array

 
// actual status
if ($order != 'user_id' && $order != 'user_name' && $order != 'user_email' && $order != 'user_type' && $order != 'registered' && $order != 'logins' && $order != 'last_login' && $order != 'user_lock' && $order != 'user_hp' && $order != 'email_contact' && $order != 'online') $order = 'user_name';
 
// what I'm intended to reach
$orderValues['users_list'] = array('user_id', 'user_name', 'user_email', 'user_type', 'registered', 'logins', 'last_login', 'user_lock', 'user_hp', 'email_contact', 'online');
 
if (!in_array($order, $orderValues['users_list'])) $order = 'user_name';
 

… which led me to a few questions.

Is an array of allowed values maintainable? And consequential:
- How often these lists was changed in the past?
- How often it is expectable to be changed in the (near) future?

Where such lists should be stored to keep them globally accessible for all of our scripts?

As second, i found many occurences of comparisions in a if-else-manner which are IMHO better readable in a ternary operation.

 
// I found as an example:
if (isset($_REQUEST['action'])) $action = $_REQUEST['action'];
else $action = 'main';
 
// IMHO better
$action = (isset($_REQUEST['action'])) ? $_REQUEST['action'] : 'main';
 

The ternary operator is in use at many places but because of the development over (around) 10 years, the notation of such checks is very non-uniform. Is it worth the effort to to comb through the scripts to replace the "old" with the "new" notation only for developers better readability of the scripts and a few hundred or thousand bytes less code?

Tschö, Auge

--
further development of mlf1

questions about comparisions in the PHP code base

by danielb987, Monday, February 05, 2018, 15:57 (13 days ago) @ Auge

Hello

As second, i found many occurences of comparisions in a if-else-manner which are IMHO better readable in a ternary operation.

 
// I found as an example:
if (isset($_REQUEST['action'])) $action = $_REQUEST['action'];
else $action = 'main';
 
// IMHO better
$action = (isset($_REQUEST['action'])) ? $_REQUEST['action'] : 'main';
 

The ternary operator is in use at many places but because of the development over (around) 10 years, the notation of such checks is very non-uniform. Is it worth the effort to to comb through the scripts to replace the "old" with the "new" notation only for developers better readability of the scripts and a few hundred or thousand bytes less code?

The phpBB forum has a method request_var and the rest of the code base is not allowed to use $_REQUEST directly! I strongly recommend you to use the same principle. You don't need all the features that the phpBB version of "request_var" has, but two important things is that it forces you to have a default value (so you don't need to check that in every place) and it checks the type.

The reason is security. Using $_REQUEST directy may results in bugs and those bugs is probably related to security. (If you handle user input badly you get a security hole).

Once you have this method (or something like it) in place, you can simply search the code base for $_REQUEST and replace all of them. Yes, it's a lot of work, but it will make the code more safe.

If you implement "request_var", there is two things you need:
* Force to have a default value so that the caller doesn't have to check the value.
* Check if the default value is an integer or a float. If that is the case, ensure that the value is an integer or a float, in order to protect from for example sql injection.

Regards,
Daniel

Avatar

questions about comparisions in the PHP code base

by Auge ⌂ @, Monday, February 05, 2018, 17:47 (13 days ago) @ danielb987

Hello

Please bear in mind, that the use of $_REQUEST is not the normal behaviour even it ispresent on several places. On most places the checks performs explicitely against $_GET or $_POST (or what else) to ensure the source of the variables. IMHO this has a better readability than $_REQUEST but also a better readability than a function (without a parameter to specify way of the incoming data) because in the code I can see the designated source of the data.

$foo = (!empty($_GET['bar'])) ? trim($_GET['bar']) : NULL; // ahhh, an URL-parameter

Tschö, Auge

--
further development of mlf1

questions about comparisions in the PHP code base

by danielb987, Monday, February 05, 2018, 18:02 (13 days ago) @ Auge

Hello

Please bear in mind, that the use of $_REQUEST is not the normal behaviour even it ispresent on several places. On most places the checks performs explicitely against $_GET or $_POST (or what else) to ensure the source of the variables. IMHO this has a better readability than $_REQUEST but also a better readability than a function (without a parameter to specify way of the incoming data) because in the code I can see the designated source of the data.

$foo = (!empty($_GET['bar'])) ? trim($_GET['bar']) : NULL; // ahhh, an URL-parameter

Tschö, Auge

Yes, I agree with that $_GET is preferred over $_REQUEST. But my point is that you may end up with code like the one below:

$page = (!empty($_GET['page'])) ? trim($_GET['page']) : 0; // ahhh, an URL-parameter

And that the code later assume that $page is an integer and therefore is not quoted correctly in a SQL query. Which may open up the forum for SQL injection.

Example:

$query = 'SELECT .... FROM ... WHERE page='.$page.' AND ... ';

By using a function, you will force that the parameter $page is an integer and thereby protect from SQL injection.

Kind regards,
Daniel

RSS Feed of thread
powered by my little forum