PHP: most efficient way of structuring IF clauses

Question

I'm looking at a way of structuring if clauses using the DRY principles of Don't Repeat Yourself.

This is work involving scripts that are ~15 years old and poorly coded with globals etc., and I'm asked to drag this script/site into the 21st Century - but due to time and cost constraints, I can not facilitate a complete site rewrite from scratch, although I know that would be far better.

I have a value that is formerly a global value and I do not know where it comes from and it may come from different places from different pages.

I have some activity that is checking an input value in $_POST or $_GET data, if the input value is empty (or invalid), then check if the value is in fact sat in a $_SESSION. If the input value is still empty (or invalid) then boot to another page.

My code as it stands:

$userId = $_REQUEST['userid'];
if (empty($userId)) {
    $userId = $_SESSION['userid'];
    }
if(empty($userId) || !is_numeric($userId))
    {
    header("Location:contactdetails.php");
    die();
    }

I repeat the empty() function twice, I could wrap both IF's into one line but then would need an IF to pass the value from the REQUEST or the SESSION into the $userId variable.

Is there a (better) way that I can check the two possible inputs to see where this [formerly global] '['userid']' variable is coming from and applying that value to the page-local userId variable?

Answer 1

You can use the ternary operator. The first expression will be used if it evaluates to true, otherwise the latter one. The $_REQUEST superglobal takes precedence in this case, like the code in the question:

$userId = $_REQUEST['userid'] ?: $_SESSION['userid'];
if (empty($userId) || !is_numeric($userId)) {
    header("Location:contactdetails.php");
    exit;
}

However as Havenard stated in a comment above, blindly trusting request data could be a security issue.

Also note that the condition will be true if any user IDs are 0, in that case a null check would be better:

$userId = $_REQUEST['userid'] ?: $_SESSION['userid'];
if ($userId === null || !is_numeric($userId)) {
    header("Location:contactdetails.php");
    exit;
}

Of course this is assuming that you do not store falsy values in the $_SESSION as a non-null value.

Answer 2

If $_SESSION['userid'] is guaranteed to be set, rink.attendant.6's answer seem like a clean approach. Otherwise, you will have to perform the necessary checks for both $_REQUEST and $_SESSION to guarantee that $userId is set properly:

if (isset($_REQUEST['userid']) && is_numeric($_REQUEST['userid']))
    $userId = $_REQUEST['userid'];
else if (isset($_SESSION['userid']) && is_numeric($_SESSION['userid']))
    $userId = $_SESSION['userid'];
else // no acceptable value for $userId in sight
{
    header("Location: contactdetails.php");
    exit;
}

You might want to reconsider using is_numeric() also, since it validates as true for numeric representations in any format, not just positive integers as you might expect.

Answer 3

There are two different problems you're solving here. First is the problem of defaulting and second is filtering. Take each in turn.

For defaulting, you can implement a simple "get from array if it exists otherwise default" helper:

function array_get(array $a, $key, $default = null) {
    return (array_key_exists($key, $a) ? $a[$key] : $default);
}

You can then use this helper to provide default chaining:

$userId = array_get($_REQUEST, 'userid', $_SESSION['userid']);

For filtering, you know from this chain that you've got either a null or a value from one of the two arrays. Since you're looking for ostensibly a database ID, I like a function like this:

function is_id_like($it) {
    $rc = filter_var($it, FILTER_VALIDATE_INT, array ('options' => array (
        'default' => false, 'min_range' => 1,
    )));
   return (false === $rc ? false : true);
}

This ensures that the number you give it looks like an int, is 1 or higher, and will return false if not. So all these pass: 1, "1", and "1.0" but these all fail: 0 and "1.1".

---

Combining these, and allowing for the session to not have a user ID:

$userId = array_get($_REQUEST, 'userid', array_get($_SESSION, 'userid'));
if (! is_id_like($userId)) {
    header('Location: contactdetails.php');
    die();
}

The total number of checks has changed to one array_key_exists and one filter_var, but the code is substantially more readable and these methods can be reused throughout your code base.

Answer 4

If the value will only ever be set in either the request or session then concat the possible values and validate once.

Answer 5

You should be using if-else for two-way statements and using a switch for n-way statements.

swicth($myVar){
case 1: doX();break;
case 'a': doY();break;
case 2: doZ();break;
default: doA();
}