TalkPHP - View Single Post

Haris · 10-04-2007, 07:55 PM

Quote:

Originally Posted by bluesaga

Code:

$szSQL = "SELECT id FROM users WHERE user = '$szUser' LIMIT 0,1";

Would be better as:

Code:

$szSQL = sprintf("SELECT id FROM users WHERE user = '%s' LIMIT 0,1", mysql_escape_string($szUser));

Its more secure to use that.

Code:

            if($bResult == True){

There is no need to use the "== True" just use:

Code:

               if($bResult){

Other than that, the code looks good, but i haven't looked THAT hard at it lol

Thanks and I thought sprintf was only used for user submitted data?

Quote:

Originally Posted by Salathe

When you're checking that the visitor is authorised to view the page, if the check fails you should display the message and stop the execution of the script immediately. That way you can get rid of the else safely and know that the script will run only if logged in.

Yours

PHP Code:


			
    //

    // Check if user is logged in

    //

    

    if($auth->check() == false || $auth->admin_auth() == false){

        echo 'Please login as administrator';

    }

Suggested alternative

PHP Code:


			
    //

    // Check if user is logged in

    //

    

    if($auth->check() == false || $auth->admin_auth() == false){

        //echo 'Please login as administrator';

        $tpl->assign('szErrorMessage', 'Please log in as administrator.');

        $tpl->display(ADMINTEMPLATE_PATH.'error.tpl.php');

        exit; // I don't know if $tpl->display() stops the script, so exit here.

    }

I haven't looked below the first else yet, so there may be more comments to follow. :)

Thanks Salathe. Saves two bunch of lines.