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?

formerly php version 4, now the target php version is 5.5.17 — Mm-Art-In
– Mm-Art-In, Commented Jun 9, 2015 at 17:41
If it can come from multiple spots (all being valid) wouldn't it be more efficient to coalesce it in one line? — Brad Christie
– Brad Christie, Commented Jun 9, 2015 at 17:44
Sometimes it cannot be helped, but something smells fishy on this code anyway. Why are you trusting $userId from the client? It looks like something that should be kept internal, since you are envolving $_SESSION['userid'] on the thing. — Havenard
– Havenard, Commented Jun 9, 2015 at 17:44
@BradChristie that is what I'm asking, can I do that while still preserving which incoming variable (SESSION or REQUEST) is sending the value? REQUEST variables trump SESSION variables as far as I can tell with the code — Mm-Art-In
– Mm-Art-In, Commented Jun 9, 2015 at 17:45

rink.attendant.6 · Accepted Answer · 2015-06-09 21:23:16Z

3

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.

edited Jun 9, 2015 at 21:23

answered Jun 9, 2015 at 17:47

rink.attendant.6

46.7k64 gold badges111 silver badges161 bronze badges

Sign up to request clarification or add additional context in comments.

7 Comments

Mm-Art-In Over a year ago

This looks like what I have been looking for, thank you. In reply to your statement on trust, yes, I agree entirely but I use the is_numeric function to try and limit the risks here.

rink.attendant.6 Over a year ago

@Martin Do note that is_numeric allows you to enter numbers in other forms such as hexadecimal and scientific notation. ctype_digit might be better for this case.

Mm-Art-In Over a year ago

Thanks for the heads-up on that, I will put that in place.

venca Over a year ago

this will generate E_NOTICE if $_REQUEST['userid'] not set

Armage Over a year ago

Hope you'll never have a userId == 0, my client have (too bad, I know).

|

Community · Accepted Answer · 2017-05-23 11:43:40Z

3

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.

edited May 23, 2017 at 11:43

CommunityBot

11 silver badge

answered Jun 9, 2015 at 17:53

Havenard

28.1k5 gold badges38 silver badges66 bronze badges

1 Comment

Mm-Art-In Over a year ago

The primary input for the values will be a GET or POST , the SESSION data would be only on a few pages.

bishop · Accepted Answer · 2015-06-09 18:02:19Z

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.

NicoE · Accepted Answer · 2015-06-09 17:45:41Z

0

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

answered Jun 9, 2015 at 17:45

NicoE

1867 bronze badges

1 Comment

Brad Christie Over a year ago

Based on the wording, it looks like its possible for1 or more locations to contain a valid value, each having its own precedence.

Kirk Powell · Accepted Answer · 2015-06-09 17:47:01Z

-2

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();
}

answered Jun 9, 2015 at 17:47

Kirk Powell

9089 silver badges14 bronze badges

Collectives™ on Stack Overflow

PHP: most efficient way of structuring IF clauses

5 Answers 5

7 Comments

1 Comment

Comments

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

7 Comments

1 Comment

Comments

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related