questions about comparisions in the PHP code base

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

--
Trenne niemals Müll, denn er hat nur eine Silbe!

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

Hello

Please bear in mind, that the use of $_REQUEST is not the normal behaviour even it is present at 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, a URL-parameter

Tschö, Auge

--
Trenne niemals Müll, denn er hat nur eine Silbe!

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

my little forum

questions about comparisions in the PHP code base (Technics)