www.webdeveloper.com
Results 1 to 5 of 5

Thread: problem with if statement

  1. #1
    Join Date
    Dec 2013
    Posts
    57

    problem with if statement

    I am trying to have an if statement appear if a user is logged in. Here is the code
    PHP Code:
    <?php 
    require 'classLogin.php';

    ?>
    <!DOCTYPE html>
    <html lang="en">
      <head>
    <title>Login</title>
      </head>

      <body>

        <div class="container">

          <form method="post" action="form.php">
            <h2>Please sign in</h2>
            <input type="text" class="form-control" name="user_name" placeholder="Email address" value="<?php if(isset($_POST['user_name'])) { echo ($_POST['user_name']);} ?>" >
            <input type="password" class="form-control" name="user_password" placeholder="Password" autofocus>
            <button name="submit" type="submit">Sign in</button>
          </form>
    <?php 
    $classLogin 
    = new Login;


    if(isset(
    $_POST['submit'])) {
    $classLogin->loginWithPostData();
    }

    if (
    $user_is_logged_in true) {
    echo 
    "USER IS LOGGED IN";
    }
    ?>
        </div>




      </body>
    </html>
    PHP Code:
              public $user_is_logged_in false;



    public function 
    loginWithPostData()
    {
    if ((
    $query->num_rows == 1) && ($_POST['user_password'] === $result->user_password)) {
     
     
                echo 
    "You are now logged in " $this->user_name "";
                return 
    $this->user_is_logged_in true;
                         } 

    For some reason the echo "USER IS LOGGED IN" appears no matter what I type in the statement such as "
    PHP Code:
    if ($user_logd_in tr
    "

    Ive been stuck for a few hours now so hopefully somebody can determine the issue and hopefully show an efficient way to do something like this.

  2. #2
    Join Date
    Oct 2012
    Posts
    42
    You are setting the value of $user_is_logged_in, instead of testing if it is true or not - the if condition will always return true so the message 'USER IS LOGGED IN' will always be displayed.

    try if($user_is_logged_in == true)

  3. #3
    Also in addition to the == instead of = (@rh_lloydnorthov, good catch!) I'd also point out that $user_is_logged_in APPEARS to be a property of whatever class you declared it public of -- which I'll ASSUME (and that's the problem with incomplete snippets) is $classLogin

    Which is why that should probably actually be:

    if ($classLogin->user_is_logged_in) {

    Note, if it's returning boolean (true/false) you shouldn't even have to say ' == true' or '== false' -- the only reason to even have == on a boolean would be === when you return false or a non-false value.

    That said, since it's unlikely you'd have more than one user logged in per execution, I'd look at making that class a singleton so you don't have to 'new' it and a code elevation couldn't make a new version -- likewise I'd use a getter method instead of making the value public (so a code elevation can't screw with the value)...

    Oh, and cute trick I usually use in my code, instead of a boolean for logged in, I return the user id from the database; if they aren't logged in? Return 0. Then to test for logged in you can just do "if (user::getId())" -- since 0 is treated as false.

    You also appear to be pulling the password from the database to compare, instead of comparing IN the query -- not good; when possible try to keep password directions mono-directional, going towards the database and never, EVER pulling from the database.

    See my usual login check query:

    Code:
    // assumes $db is a connected PDO object
    
    $statement = $db->prepare('
      SELECT id, username FROM users
      WHERE username = :username
      AND password = :password
    ');
    
    $statement->execute([
      ':username' => $_POST['username'],
      ':password' => hash('sha256', $_POST['password'])
    ]);
    
    unset($_POST['password']); /* remove it from global space ASAP */
    
    if ($user = $statement->fetch()) {
      // login successful, copy $user to 'this' 
      // and pull user specific info from userInfo table
    } else {
      // login failed, set user as guest and re-send form
    }
    Password check done sql side, not client side -- destroy the password the moment we are done with it -- some people say I'm overly paranoid about security. I say there's no such thing.

  4. #4
    Join Date
    Dec 2013
    Posts
    57
    Quote Originally Posted by deathshadow View Post
    Also in addition to the == instead of = (@rh_lloydnorthov, good catch!) I'd also point out that $user_is_logged_in APPEARS to be a property of whatever class you declared it public of -- which I'll ASSUME (and that's the problem with incomplete snippets) is $classLogin

    Which is why that should probably actually be:

    if ($classLogin->user_is_logged_in) {

    Note, if it's returning boolean (true/false) you shouldn't even have to say ' == true' or '== false' -- the only reason to even have == on a boolean would be === when you return false or a non-false value.

    That said, since it's unlikely you'd have more than one user logged in per execution, I'd look at making that class a singleton so you don't have to 'new' it and a code elevation couldn't make a new version -- likewise I'd use a getter method instead of making the value public (so a code elevation can't screw with the value)...

    Oh, and cute trick I usually use in my code, instead of a boolean for logged in, I return the user id from the database; if they aren't logged in? Return 0. Then to test for logged in you can just do "if (user::getId())" -- since 0 is treated as false.

    You also appear to be pulling the password from the database to compare, instead of comparing IN the query -- not good; when possible try to keep password directions mono-directional, going towards the database and never, EVER pulling from the database.

    See my usual login check query:

    Code:
    // assumes $db is a connected PDO object
    
    $statement = $db->prepare('
      SELECT id, username FROM users
      WHERE username = :username
      AND password = :password
    ');
    
    $statement->execute([
      ':username' => $_POST['username'],
      ':password' => hash('sha256', $_POST['password'])
    ]);
    
    unset($_POST['password']); /* remove it from global space ASAP */
    
    if ($user = $statement->fetch()) {
      // login successful, copy $user to 'this' 
      // and pull user specific info from userInfo table
    } else {
      // login failed, set user as guest and re-send form
    }
    Password check done sql side, not client side -- destroy the password the moment we are done with it -- some people say I'm overly paranoid about security. I say there's no such thing.
    Thanks a lot guys. I changed it to "===" and now it is working properly.

    Also, is it best practice to use PDO to connect to db or is it fine the way I am doing it with oop.

  5. #5
    You didn't actually show how you are connecting, so I have no clue what your $query object was.

    I usually extend the PDO object myself -- making it call the connection info itself with a check so it can only initialize once from my 'one index.php to rule them all'; restricting the $db var's scope as the only allowed instance of PDO so that only code that should be able to use the database can do so (reducing the windows in which code elevations can occur)...

    I even override :repare, ::exec and ::query with the ability to pass a $module and $query strings, which lets me use 'named queries' loaded from a $module.ini file (which are kept local in scope when loaded to my extended PDO object) so code can't just 'make up' queries out of the blue. You have to add them to the appropriate .ini file.

    ... which also means I can load different .ini files depending on the engine.

    for example:

    $db->exec('purgeExpiredSessions', 'common');

    Would load /queries/engineName/common.ini if it's not already in the array of queries (pdoExtended::queryStrings['common']), and then parent::exec($this->queryStrings['common']['purgeExpiredSessions']);

    The neat part being that doing it this way lets me have:
    /queries/mysql/common.ini
    /queries/mssql/common.ini
    /queries/postgresql/common.ini
    /queries/sqlite/common.ini

    ... and so forth. I also store my db.php file (containing the extended objects) in /queries/engineName so I can have different codebases of the object for each engine, letting me 'polyfill' engine differences. (which can normally be a PITA)

    The technique is called "named queries" -- was really big in the mid 1990's when suddenly there were all these new engines people wanted to use in existing languages, but faded into obscurity for... well, I'm not sure why.

    In any case (yeah, I know, I'm rambling. Insomnia does that) without seeing what you are using FOR an object, it's hard to say if you're doing it right or not. If your object is WRAPPING PDO, that might be a waste, if however you are extending it? Usually not a bad idea. After all, extensibility is one of the entire reasons to use objects in the first place.

    Though if your object is wrapping the old mysql_ functions -- do yourself a favor and stop right there.

Thread Information

Users Browsing this Thread

There are currently 1 users browsing this thread. (0 members and 1 guests)

Tags for this Thread

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  
HTML5 Development Center



Recent Articles