CODEWITHSUNDEEP
php
BDuelz
BDuelz

Reputation: 3928

Redundancy in PHP? safer or unnecessary

I'm using the MVC model (I think thats what its called) and I have separated my site into smaller pages and includes.... Is it safer/better or worse (with no benefit) to check the same conditional twice?

For example, I have an accounts page that looks something like this:

// Must be logged in
if(isset($_SESSION['userID'])){

    include('edit_user.php');

}

and then in my edit_user.php page I have something like this:

// Must be logged in
if(isset($_SESSION['userID'])){

    if(isset($_POST['editUser'])){
        //Validate the form
    }
?>

<form>
// Display the form
</form>

<?php
} // End main IF

So pretty much I'm checking if the user id is set twice... I'm pretty mush doing the same thing with all my pages (that require users be logged in). Is that really necessary? My initial thought was to prevent unregistered users from accessing the edit_user.php form directly and doing things (I was also thinking of just redirecting if users do access the page directly). What do you guys think/suggest?

Edit

I dont think I explained myself too clearly... That was just an example... Here's a better example to better get across the reasons for my question:

...Account page

if(isset$_SESSION['userID'])){

    include('edit_user.php');// edit user form

    include('change_password.php');// change password form

    include('change_pic.php');// change photo form

}

and from within each of my includes, again I'm asking for a SESSION['userID']... So, what do you guys suggest now?

Upvotes: 1

Views: 268

Answers (5)

Brandon Hansen
Brandon Hansen

Reputation: 826

You shouldn't need to have the multiple checks in there. If all requests go through the controller, then you should only need to add the check in the controller. That is the point of the controller, to direct the request. The view outputs data. The model interacts with the database (and enforces business logic).

Upvotes: 1

DisgruntledGoat
DisgruntledGoat

Reputation: 72510

Joomla uses a nice method, which is to put this right at the top of each PHP file:

defined('_JEXEC') or die('Restricted access');

_JEXEC is defined in the main entry point. You could do something like:

if ( isset($_SESSION['userID']) ) {
    define('LOGGED_IN', 1);
    include('edit_user.php');
}

With this in edit_user.php and other files:

defined('LOGGED_IN') or die('You must be logged in.');

Upvotes: 0

James Black
James Black

Reputation: 41858

You should always validate input that comes in from outside your control, so every php script that can be accessed by a user should check.

Even if you don't expect it, if a user can see a link in the url to a page that is 10 forms deep in your site then they can still jump directly to it.

Upvotes: 0

Steven Mercatante
Steven Mercatante

Reputation: 25295

In many cases redundancy is beneficial, but in your example it's unnecessary. It also goes against the principle of DRY (Don't Repeat Yourself). The more you repeat the same code, the more time you waste (you also face the possibility of adding errors to your code due to repetition). You should be fine with the check solely in edit_user.php.

Since you're using MVC, here's what I suggest: Define some authentication functions that are globally accessible. Then, in your controller's constructor methods, use them to see if the user should be granted access to that section. If they're not authorized you can redirect them to another page, or display an error message, for example. Of course you can use a finer-grained methodology and place the calls to your authentication functions in the beginning of certain controller methods.

Upvotes: 0

zombat
zombat

Reputation: 94147

Well, it is redundant, which violates the "Don't Repeat Yourself" (DRY) principle of design. If your edit_user.php file is publicly accessible, then you definitely need checks in there, so you could probably remove the other checks, as long as you're sure of the functionality.

It's arguable that your code is clearer with the checks in place, however, in the long run redundancy like that will lead to more maintenance hassles.

Upvotes: 2

Related Questions