#1
  1. A Change of Season
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Mar 2004
    Location
    Next Door
    Posts
    2,664
    Rep Power
    171

    Functions should not rely on user input data


    Functions should be given as much information as they need directly without relying too much on other data sources (such as $_GET/POST), with some exceptions.
    1 - Does this mean for every function that expects POSTed or GETed values, it is necessary to set default function arguments values as well because otherwise PHP shows Warning missing argument in case the user won't pass any data?


    2 - Is something like this considered a good design?
    PHP Code:
    function log_in($email=NULL$password=NULL)
        {
            if(
    $email==NULL || $password==NULL)
                {
                    return 
    false;    
                }
            else
                {
                    .....
                }

             } 
  2. #2
  3. Did you steal it?
    Devshed Supreme Being (6500+ posts)

    Join Date
    Mar 2007
    Location
    Washington, USA
    Posts
    14,066
    Rep Power
    9398
    I feel like I shouldn't weigh in because you're quoting something I said, but I will anyways.

    Imagine the log_in function is a controller. It is, basically: it takes input from somewhere (the model) and probably won't output anything (the view).
    As such it should stay separate from the model: it doesn't really care where the data came from so long as it has an email address and a password. Did it come from $_POST? Probably as that's the primary use case, but it could be somewhere else. By assuming it's always $_POST you're tightly coupling the function to the input and if something changes in the future then you have to rewrite that, and not just the function but anywhere the function is called.

    So do it right to begin with: whatever is calling the function knows where the data is, so let it pass the data in.

    Which doesn't actually address your questions.

    1. Default values only affect the literal invocation of the function. If the calling code does not do due diligence and calls
    PHP Code:
    log_in($_POST["email"], $_POST["password"]); 
    without checking that the values are present in $_POST, $email and $password will be null because that's the value of $_POST["email"] and $_POST["password"]. The only way to trigger default values for parameters is to not pass anything at all as an argument:
    PHP Code:
    log_in() 
    Now, does it make sense to call the function that way? No. Logging in requires an email and a password and not giving either is ridiculous. By providing defaults you imply that either is optional when they are both necessary.

    2. Default arguments aside, it is not harmful to check that $email and $password have values. Strictly speaking the code calling log_in should verify that there are email and password values, but log_in should itself verify the values.

    There's one issue though: how do you differentiate between missing values and an invalid login (which would return false as well)? 10:1 odds that Jacques will tell you to use exceptions and I actually agree with that: they'd allow you to know exactly why this function failed.
    PHP Code:
    function log_in($email$password) {
        if (
    $email == "" || $password == "") {
            
    // the "domain" of this function is non-empty emails and passwords
            // (if you remember from math class, the domain of a function is its input
            // and the range is its output)
            // violating that precondition results in a DomainException
            
    throw new DomainException("Email or password is empty"); // provided by SPL
        
    }

        
    // try to log in
        // if credentials don't match {
            // credentials not matching is a normal condition
            // IMO exceptions are for abnormal conditions and thus not appropriate here
            
    return false;
        
    // } else {
            
    return true;
        }

    PHP Code:
    try {
        if (
    log_in($_POST["email"], $_POST["password"])) {
            
    // logged in successfully
        
    } else {
            
    // invalid credentials
        
    }
    catch (
    DomainException $de) {
        
    // invalid input(s)

    You can actually go even more detailed for the different error conditions:
    - Email or password is empty
    - Email is not an email
    - Email does not exist Your function may do this but do not reveal this information to the user - if the email doesn't exist then it should be the same message you give if the password is incorrect
    - Email and password do not match a user, aka incorrect password
    in which case you should use different exceptions, such as DomainException, InvalidArgumentException, "NoSuchUserException", and "InvalidCredentialsException" (the last two being your own exception classes), so you can distinguish between the different types of errors.
  4. #3
  5. A Change of Season
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Mar 2004
    Location
    Next Door
    Posts
    2,664
    Rep Power
    171
    Before replying to your comments I should check these with you. Please tell me what is wrong with each item below:

    try, catch.

    So many different tutorials on this! A lot of different points of views and confusion between different posters and those who reply to their comments, that kinda thing that makes Jaques1 angry! Ok please lets clarify:

    try catch is for error handling in special situations. In order for the code to handle the errors with crashing.

    How it works:

    I donnot how to say it but "try" is always called. Like echo functionName(); So whenever is inside try gets executed!
    Somewhere further there will be a thrwo new Exception with a message inside of it. This does not neccessarily get thrown.
    Try has to be followed by a catch to show the error in case there is one. Basically:
    PHP Code:
    try
        {
            
    /*
            
            1 - Either if / else inside try
            if($x!=$y)
                {
                    throw Exception('Bad news!');    
                }
            
            ...................................................
            
            2 - Or call a function
                check_this($username); //Check_this is below
            
            
            */
        
    }
    catch(
    Execption $e)
        {
            echo 
    $e->getMessage();    
        } 
    PHP Code:
    //Somewhere above:
    function check_this($val)
        {
            if(
    $val=='')
                {
                    throw new 
    Exception('Bad news');
                    return 
    false;
                }
            else
                {
                    
    header('Location: http://www.example.com');    
                    exit();
                }
        } 
    Is it correct so far?


    To apply this to the log_in code, perhaps something like this:

    PHP Code:

    function log_in($email$password)
        {
            if(
    $email == "" && $password == "")
                {
                    throw new 
    Exception('Empty values');
                }
            if(
    $email == "" && $password != "")
                {
                    throw new 
    Exception('Empty email');
                }
            if(
    $email != "" && $password == "")
                {
                    throw new 
    Exception('Empty password');
                }
            if(!
    filter_var($emailFILTER_VALIDATE_EMAIL))
                {
                    throw new 
    Exception('Invalid Email');    
                }
            
            
            
            
    $stmt $connection->prepare("SELECT id from website_test_members WHERE email = :email");
            
    $stmt->execute(array(':email' => $email));
            
    $count $stmt->rowCount();
            if(
    $count!=1)
                {
                    throw new 
    Exception('Invalid Credentials');    
                }    
                
        }
    try
        {
            if(
    log_in($email$password))
                {
                    
    header('Location: http://www.example.com');    
                    exit();
                }
            else
                {
                    echo 
    "Invalid";
                }
        }
    catch(
    Exception $e)
        {
            echo 
    $e->getMessage();
        } 
    Or do something totally different
    PHP Code:
    if($_POST['submit_log'])
        {
            if(
    filter_var($_POST['email'], FILTER_VALIDATE_EMAIL) && strlen($_POST['password']>0))
                {
                    if(
    log_in($_POST['email'], $_POST['password'])) //Check with the database and return true or false
                        
    {
                            
    header('Location: http//www.profile.com');
                            exit();
                        }
                    else
                        {
                            
    $invalid_crdentials true;    
                        }
                }
        } 
    Thank you
  6. #4
  7. Did you steal it?
    Devshed Supreme Being (6500+ posts)

    Join Date
    Mar 2007
    Location
    Washington, USA
    Posts
    14,066
    Rep Power
    9398
    Originally Posted by English Breakfast Tea
    Is it correct so far?
    Yes: what's in the try block is always executed until an exception is thrown, at which point that code stops and what's in the corresponding catch block is executed instead. It doesn't matter where the exception is thrown (so long as it happens during execution of that try block, of course) so your code, or a function, or a function 10 calls down the stack could throw one.
    PHP 5.5 adds the long overdue "finally" block, which always executes at the end regardless of whether the try block completed or a catch block executed.
    PHP Code:
    try {
        echo 
    "I executed\n";

        throw new 
    Exception();

        echo 
    "I will not execute\n";
    } catch (
    ASpecificException $ase) {
        echo 
    "I will not execute because it is a vanilla Exception\n";
    } catch (
    Exception $e) {
        echo 
    "I will execute because the exception thrown was an instance of Exception\n";
        
    // (as all exceptions are)
    finally {
        echo 
    "I will always execute\n";
    }

    // I executed
    // I will execute because the exception thrown was an instance of Exception
    // I will always execute 
    Originally Posted by English Breakfast Tea
    To apply this to the log_in code, perhaps something like this:
    Perhaps. Three things:

    1. Exceptions are most powerful when you use different types of them for different conditions. If you always use Exception then calling code has no possible recourse for dealing with what are, really, varying degrees of errors. In your example there are three: (1) invalid function call because there are missing values and calling code should know better than to do that, (2) invalid argument because the $email was supposed to be an email address but wasn't, and (3) the function was called with valid inputs but was unable to perform the expected action because the credentials are wrong. Each of those should get their own subclass of Exception so that my hypothetical code could (1) display a general-purpose "error" message and do something to notify me that the code is wrong, (2) tell the user that the email was not a valid email, or (3) tell the user their credentials were wrong.

    2. Given that the first three branches of your initial if fit into the #1 "the developer did something wrong by allowing information so obviously incorrect to pass through to my function", just collapse those down to one check.

    3. Style choice: since your log_in function doesn't technically require that the $email be an email address (it's not like it tries to send an email or locate a "@" or something), you could do away with that check entirely and know that the SQL will not find a match. If the calling code wants to verify the input is an email address then it can.

    PHP Code:
    function log_in($email$password) {
        if (
    $email == "" || $password == "") {
            throw new 
    Exception("Email or password is empty");
            
    // or DomainException, I don't even know anymore
        
    } else if (!filter_var($emailFILTER_VALIDATE_EMAIL)) {
            throw new 
    InvalidArgumentException("Invalid email address");
        }

        
    // ...blah...
        
    if ($count != 1) {
            throw new 
    InvalidCredentialsException("Email or password is incorrect");
        }

        return 
    true;

    PHP Code:
    $error "";
    try {
        
    log_in($_POST["email"], $_POST["password"]);
        
    header("Location: wherever");
        exit;
    } catch (
    InvalidCredentialsException $ice) {
        
    $error $iae->getMessage();
    } catch (
    InvalidArgumentException $iae) {
        
    $error $iae->getMessage();
    } catch (
    Exception $e) {
        
    $error $e->getMessage();
        
    alert($e->getMessage(), $e->getStackTrace()); // email me about this error

    When I get the alert about the email/password being empty, I'd see that in my code I didn't bother checking if the email or password were given and then change it so it does.

    There's a sliding scale of what you can check and when, but I personally subscribe to the "check when/where it's needed" mentality, such as
    - When you want to vary your actions based on the input (like giving a special message for missing input and another for invalid email). This would happen in the calling code because that's where the special message would come into play
    - When invalid input can cause problems or unexpected behavior (like needing to send an email to an address or doing a DNS lookup on the address's hostname)
    Applying that to your specific situation, I'd only throw an exception when the credentials are wrong - an invalid input is not only the user's fault but totally harmless to your code - and let the calling code check for empty inputs. (Valid email address being completely ignored.)

    But it's your code so you pick what feels right to you. I actually worked with someone who was absolutely paranoid about every possible mistake or error and while their code was very reliable it was such a hassle to read through.
  8. #5
  9. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,959
    Rep Power
    1014
    The try-catch statement actually comes from object-oriented languages like Java. It's completely different from how PHP usually handles errors, and it's not really integrated into PHP yet. That's probably the reason why so many people struggle with it.

    The main purpose of try-catch is to handle errors in a smart way.

    In classical PHP, you check the return value of a function to see if it failed:

    PHP Code:
    <?php

    $db_connection 
    mysql_connect(...);
    // check if connection failed
    if (!$db_connection) {
        die(
    'Sorry, there was a technical issue yada yada yada');
    }
    That's OK for procedural code. If your whole script is just one big file with a bunch of statements, then this is how you do it.

    However, modern web applications tend to be a bit more complex than the amateur home pages from the 90s. You may have a lot of classes, deeply nested method calls and multiple layers of abstraction. In this scenario, the old way of handling errors no longer works. What if you want to handle an error at a higher level instead of reacting to it right away? What if you want to handle a whole group of errors? You simply can't do that with return values.

    Exceptions are smarter: They represent the error as an object of a specific error class like PDOException or DatabaseException or simply Exception (which is the most generic class). The exception "bubbles up", which means it gets passed on from method to method. Any method is allowed to catch the exceptions of a certain class and handle them. If you don't catch an exception at all, it "bubbles up" to the top level and finally generates a fatal error.

    You do not have to catch an exception. If fact, you should not unless you actually wanna do something with it. A common misconception is that every piece of code needs to be wrapped in a try-catch block to do something like this:

    PHP Code:
    <?php

    try {
        
    // whatever
    } catch (Exception $e) {
        die(
    $e->getMessage());
    }
    No! This is complete nonsense. And it's a shame that the PHP manual contains a lot of examples doing this.

    Remember when I talked about smart error handling? The code above isn't smart. It simply stops the whole script right where the error happens and dumps the raw error message on the screen. The application has no chance of handling the exception at a higher level and possibly recovering from it.

    Even worse: The error message will always be displayed on the screen, regardless of whether the error happens on your local test server or on your production site with 100,000 visitors per second.

    This is an anti-pattern. It's one of those things people blindly copy and paste without ever thinking about it. As a simple rule: Do not catch an exception unless you actually wanna handle it. If you don't know what do to with the exception, simply let it bubble up. If it's not caught at all, it will automatically send the error message to the right location (the screen, a log file, whatever).

    So please erase that try {} catch (Exception $e) {die($e->getMessage())} stuff from your brain.

    So much for that.

    Regarding your (or rather requinix') code, I'm not really sure if this is a good application of exception. Is a user entering wrong data really an application error? Or isn't that rather something perfectly normal?

    Personally, I would simply save the error messages in some array and display them to the user. I wouldn't use exceptions, because I feel they don't really fit and make the code cumbersome. But let's not split hairs here. Use whatever you find most intuitive.
    Last edited by Jacques1; September 11th, 2013 at 09:50 PM.
    The 6 worst sins of security ē How to (properly) access a MySQL database with PHP

    Why canít I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".
  10. #6
  11. A Change of Season
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Mar 2004
    Location
    Next Door
    Posts
    2,664
    Rep Power
    171
    I wasn't a fan of try catch and now this is gonna take a while for me to realise what's up.
  12. #7
  13. Did you steal it?
    Devshed Supreme Being (6500+ posts)

    Join Date
    Mar 2007
    Location
    Washington, USA
    Posts
    14,066
    Rep Power
    9398
    What my use of catching Exception didn't explain was that, at that point, any thrown exceptions were being acted upon. It's as Jacques said: you don't catch everything, just the things you want to deal with, and that code was dealing with it. However I can think of a couple, fairly rare reasons why you might want to catch everything:
    PHP Code:
    $bar calculate_bar($foo);
    try {
        
    stuff($bar);
    } catch (
    Exception $e) {
        
    // maybe you want to wrap this exception in a new one
        // (perhaps you want to provide a more specialized exception with more specific
        // information about what was happening when the error occurred)
        
    throw new Exception("Could not execute stuff() using foo={$foo}"0$e);
        
    // (third argument being the previous/inner exception)

        // or maybe there's some weird case where you may or may not want to handle
        // exceptions
        
    if ($handle_this_exception) {
            
    // whatever
        
    } else {
            throw 
    $e;
        }
        
    // but I can't think of any reason off the top of my head why you might want to
        // do that here and not further up the call stack

    The only place where you would (potentially) catch all exceptions is in some sort of front controller location where all code branches out from. Like those frameworks where you route all requests through a single file? That file could have a try/catch in there.
    The reason is that an uncaught exception will completely kill the script. As if you die()d. Naturally that's not a good user experience so you'd catch all exceptions at that chokepoint and then dump out some simple (to avoid even more errors) "internal server error"-type page. It's not great, but it's better than a white screen of death.

    If you don't have that single file, or can't otherwise use a try/catch block, there's set_exception_handler.

    Originally Posted by Jacques1
    Regarding your (or rather requinix') code, I'm not really sure if this is a good application of exception. Is a user entering wrong data really an application error? Or isn't that rather something perfectly normal?
    Yeah, I know. Depends how you look at it: either you violated a precondition by passing bad data (which is, IMO, a valid but extreme way of looking at it) or you need to run the input through some error checking before you try to act on it. Exceptions work a lot better with the former than the latter, but I didn't want to come up with a different example to explain exceptions. Plus I kinda had suggested to him using them for this purpose earlier so...

    Comments on this post

    • Jacques1 agrees
  14. #8
  15. No Profile Picture
    Contributing User
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Dec 2004
    Posts
    2,989
    Rep Power
    375
    so jacques let me get this straight, if i have a class file for example,

    i wouldnt do something like:

    PHP Code:
     class MyClass 
         
    ... some variables

        
    function test () {
            try {
                  ...
    something
           
    } catch (Exception $e) {
                   ... 
    put this into my logging mechanism
                  $this
    ->error $e->error;
                  return 
    false;
           }
      } 
    but instead do try/catch in teh scritp that calls this class?

    My reasoning for doing this in the class would be that I dont have to try to remember/do try/catch every time a script calls this class? instead i can just rely on the $error variable of class to see what happened?
  16. #9
  17. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,959
    Rep Power
    1014
    Originally Posted by paulh1983
    instead i can just rely on the $error variable of class to see what happened?
    You're bypassing the whole exception model and replacing it with your own error mechanism which is basically just a fancy variant of the "good" old return values.

    That's not good. First of all, it's a nonstandard approach, and it's not intuitive. This means everybody who works with your code will stumble upon it at first. Secondly, it's a "dumb" model compared to standard exceptions, because you cannot delegate or group errors. It works just like the return values. And third, it's a lot of unnecessary work, because you have to write the same code for each and every method in order to replace the native exception mechanism.

    The question is: What are you trying to do? Do you just wanna log all fatal errors? Then you don't need try-catch at all. Since you want the same action for every error, register a shutdown function via auto_prepend_file.

    How to do it is described in "The mystery of errors and exceptions" under "Setting up a custom error page".

    Note that it's not enough to simply register an error handler or an exception handler, because those will only intercept some of the runtime errors. If you wanna catch all errors (including syntax problems), you need to have PHP prepend a shutdown handler.
    The 6 worst sins of security ē How to (properly) access a MySQL database with PHP

    Why canít I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".
  18. #10
  19. No Profile Picture
    Contributing User
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Dec 2004
    Posts
    2,989
    Rep Power
    375
    how is it unnecessary work? wouldnt it be more unnecessary if i had to do try and catch in EVERY script that calls this class? instead now i can forget about this?

    so are you saying instead of that I should do something like:

    PHP Code:
    class MyClass 
         
    ... some variables

        
    function test () {       
              ..do 
    something

          
    if ( false ) {
                 throw 
    an exception....... OR
                 
    trigger_error('some message'); 
          }
      } 
  20. #11
  21. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,959
    Rep Power
    1014
    Originally Posted by paulh1983
    wouldnt it be more unnecessary if i had to do try and catch in EVERY script that calls this class?
    If you actually used this approach (which you shouldn't), then you'd only wrap every script which is directly callable by the users in a big try-catch block. Because that's where (runtime) errors eventually end up.

    How many of those scripts do you have? Maybe 10? 50? 100? That's not all too much work.

    But like I said: A central shutdown handler makes this unnecessary. It will handle each and every error so that you don't have to repeat the same actions again and again.



    Originally Posted by paulh1983
    so are you saying instead of that I should do something like:

    PHP Code:
    class MyClass 
         
    ... some variables

        
    function test () {       
              ..do 
    something

          
    if ( false ) {
                 throw 
    an exception....... OR
                 
    trigger_error('some message'); 
          }
      } 
    Not sure what you mean. Where does this false come from? Does it come from some native PHP function to signal an error? And now you wanna turn this into an exception? That wouldn't make a lot of sense, because PHP usually generates an error already if there's something wrong. I'm not aware of a function which only returns false and stays completely silent (but I could be wrong here).

    Either way, yeah, you might do this occasionally. But trying to do this for every single return value would be insane. We have to deal with the fact that PHP is a mixture of different concepts from different eras. You have a bit of procedural programming and a bit of OOP. Sometimes you get an error, sometimes you get false, sometimes you get an exception. If you don't like it and want a coherent concept, you'll have to choose a different language.
    The 6 worst sins of security ē How to (properly) access a MySQL database with PHP

    Why canít I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".
  22. #12
  23. No Profile Picture
    Contributing User
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Dec 2004
    Posts
    2,989
    Rep Power
    375
    sorry for the purpose of demo i was using arbitary code

    if ( false ) could be anything i.e. if ( $user == "" ) or if ( $x/0 ) etc..
    Last edited by paulh1983; September 12th, 2013 at 11:28 AM.
  24. #13
  25. No Profile Picture
    Dazed&Confused
    Devshed Novice (500 - 999 posts)

    Join Date
    Jun 2002
    Location
    Tempe, AZ
    Posts
    506
    Rep Power
    128
    Going back to the use of $_GET/$_POST/etc. within your code, I've taken on the habit of getting rid of those immediately in my code, putting the value into explicit local variables.

    This can be as simple as:
    PHP Code:
    $name = isset($_POST['name']) ? $_POST['name'] : "";
    $address = isset($_POST['address']) ? $_POST['address'] : "";
    ... 
    Or you can take the opportunity to perform basic validation/default-setting as well:

    PHP Code:
    $page = isset($_GET['page']) && is_numeric($_GET['page']) ? $_GET['page'] : 1
    Either way you shouldn't have much need to reference $_GET/$_POST/$_REQUEST throughout the script. This also has the benefit of allowing you to manually set the local variables for testing without making changes to the superglobals.

    Anyhow, just a suggestion for cleanliness.
    LinkedIn: Dave Mittner
  26. #14
  27. A Change of Season
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Mar 2004
    Location
    Next Door
    Posts
    2,664
    Rep Power
    171
    One thing at a time:
    Originally Posted by requinix
    Perhaps. Three things:

    1. Exceptions are most powerful when you use different types of them for different conditions. If you always use Exception then calling code has no possible recourse for dealing with what are, really, varying degrees of errors.
    Why is that? I see you explained it, but please explain again. How dos that exactly work? This may sound pretty dumb thing to say but your code is longer than mind and I don't really see why it's neccessary to have different Exceptions in this case (or other cases). What is the problem with having only 1 Exception ?

    Thanks
  28. #15
  29. Did you steal it?
    Devshed Supreme Being (6500+ posts)

    Join Date
    Mar 2007
    Location
    Washington, USA
    Posts
    14,066
    Rep Power
    9398
    You know how functions can return different values depending on how well it worked? True or false, object or null, number for an error code...

    The way to do that with Exceptions is different types of exceptions. Like InvalidArgumentException or UnexpectedValueException. If you throw Exception all the time then it's like returning false all the time: okay, great, you know there's a problem, but you don't know what it was. Was there a problem with the input? Should I tell the user they did something wrong? Is there some serious error that needs immediate attention? No way to know.

IMN logo majestic logo threadwatch logo seochat tools logo