SitePoint Sponsor

User Tag List

Results 1 to 10 of 10
  1. #1
    SitePoint Zealot
    Join Date
    May 2005
    Posts
    172
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    How can I improve this code?

    Hi, I'm trying to automate basic form processing as much as possible, is there any way to improve this code, make it faster, easier to work with, anything.

    PHP Code:
    <?php

    $required 
    = array('name''email''message');
    $errors = array();
    $data = array();

    if (
    is_array($_POST))
    {
        foreach (
    $_POST as $key => $value)
        {
            if (
    in_array($key$required))
            {
                if (empty(
    $value)) $errors[$key] = 'empty';
                else 
    $data[$key] = clean($value);
            }
            else
            {
                
    $data[$key] = clean($value);
            }
        }
    }


    function 
    clean($str)
    {
        return 
    mysql_real_escape_string($str);
    }

    ?>
    Last edited by spudz; Feb 16, 2009 at 08:43. Reason: removed encoding based on decowski's advice

  2. #2
    Web Professional
    Join Date
    Oct 2008
    Location
    London
    Posts
    862
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    You shouldn't entity encode the data before inserting into the database. Rather, you should entity encode it immeditely before displaying.

  3. #3
    SitePoint Zealot
    Join Date
    May 2005
    Posts
    172
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Yeah, good call Paul, i was trying to add a bit of extra assurance that quotes wouldn't screw with my db

  4. #4
    SitePoint Wizard silver trophybronze trophy Cups's Avatar
    Join Date
    Oct 2006
    Location
    France, deep rural.
    Posts
    6,869
    Mentioned
    17 Post(s)
    Tagged
    1 Thread(s)
    Couple of ideas:

    1
    run array_map() over the post vars with trim() first.

    2
    $errors[$key] = 'empty'

    Could you do this instead?

    $errors['empty'] = $key ;

    In case you later have $errors['invalid'] ?

    The final formatting of errors may differ from invalid attempts.

    3
    I haven't tested the robustness of this check:

    if (is_array($_POST))

    Could that be ( count( $_POST ) >= 1 ) - I prefer testing for positives rather than relying on testing whether an empty array passes that test or not.

    And what if you run my suggestion 1 before running suggesion 3?

    $_POST['email'] = " " ;

    Use var_dump a LOT.

    this seems wasteful:
    PHP Code:
                else $data[$key] = clean($value);
            }
            else
            {
                
    $data[$key] = clean($value);
            } 
    If you are running clean() on everything, just do it in one place.

    Huh,

    if count( $required ) is equal to 3, then if your count of POST vars is less than 3 then send them far, far away. (header to your home page)

  5. #5
    SitePoint Zealot
    Join Date
    May 2005
    Posts
    172
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Edited based on Cups advice.

    PHP Code:
    <?php

    $required     
    = array('name''email''message');
    $errors        = array();
    $data         = array();

    if (
    count($_POST) >= 1)
    {

        
    array_map('trim'$_POST);
        foreach (
    $_POST as $key => $value)
        {
            if (
    in_array($key$required))
                if (empty(
    $value)) $errors[$key] = 'required';
                        
            
    $data[$key] = clean($value);
        }
        
        if (!
    preg_match('/^([a-z0-9])(([-a-z0-9._])*([a-z0-9]))*\@([a-z0-9])(([a-z0-9-])*([a-z0-9]))+(\.([a-z0-9])([-a-z0-9_-])?([a-z0-9])+)+$/i'$data['email']))
        {
            
    $errors['email'] = 'invalid';
        }
    }

    function 
    clean($str)
    {
        return 
    mysql_real_escape_string($str);
    }

    ?>
    2
    $errors[$key] = 'empty'
    Could you do this instead?
    $errors['empty'] = $key ;
    In case you later have $errors['invalid'] ?
    The final formatting of errors may differ from invalid attempts.
    Writing the errors in the keys would overwrite on each iteration of the loop, so it would probably be best to use the $_POST keys in the $errors array aswell

    Huh,
    if count( $required ) is equal to 3, then if your count of POST vars is less than 3 then send them far, far away. (header to your home page)
    This seems too specialized at the moment, i'm trying to create a generic form processing template to work from. Also why would you check if $required is equal to 3, there could be a textbox for website which isn't required and if someone enters that I'd be sending someone away for no reason.

    Thanks for your advice, very helpful

  6. #6
    SitePoint Wizard bronze trophy
    Join Date
    Jul 2008
    Posts
    5,757
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Your current code can't detect whether or not the required fields are all present. While it will work if the user just doesn't fill in a value, what if they don't even send the variable at all(they edit the form source)? It won't exist in _POST at all, and your loop won't check them.

  7. #7
    SitePoint Zealot
    Join Date
    May 2005
    Posts
    172
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by crmalibu View Post
    Your current code can't detect whether or not the required fields are all present. While it will work if the user just doesn't fill in a value, what if they don't even send the variable at all(they edit the form source)? It won't exist in _POST at all, and your loop won't check them.
    so what would you suggest?

  8. #8
    SitePoint Wizard bronze trophy
    Join Date
    Jul 2008
    Posts
    5,757
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    This just makes sure the desired array keys are present in post
    PHP Code:
    if ($missing_fields array_diff($requiredarray_keys($_POST))) {
        
    print_r($missing_fields);

    Then you can later go and check the values and stuff.

  9. #9
    SitePoint Wizard silver trophybronze trophy Cups's Avatar
    Join Date
    Oct 2006
    Location
    France, deep rural.
    Posts
    6,869
    Mentioned
    17 Post(s)
    Tagged
    1 Thread(s)
    Writing the errors in the keys would overwrite on each iteration of the loop, so it would probably be best to use the $_POST keys in the $errors array aswell
    Would have been slightly more revealing if I had described it correctly ...

    $arr['empty'][] = 'email';
    $arr['empty'][] = 'password';

    $arr['incorrect'][] = 'email' ;

    var_dump( $arr['empty'] );


    This seems too specialized at the moment, i'm trying to create a generic form processing template to work from. Also why would you check if $required is equal to 3, there could be a textbox for website which isn't required and if someone enters that I'd be sending someone away for no reason.
    I suppose you are right. I honestly do a lot of pre-validation checks in JS (which dont count as real security, I know ...).

    If what arrives has clearly circumvented those checks somehow, I tend to be pretty brutal.

    I'd do the trim() first too.

    crmalibu's check seems fast and to the point. Fail early on with that if you can why bother validating 2 fields if you are only going to go ahead and execute changes with 3?

  10. #10
    SitePoint Enthusiast
    Join Date
    Oct 2008
    Posts
    72
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by crmalibu View Post
    Your current code can't detect whether or not the required fields are all present.
    Good point. Iterate over the required fields array and check isset($_POST[$requiredVar]).

    I would wrap this functionality inside of a class so It can easily be reused, and with a fair bit more messing around you'd be able to extend it easily, conforming to the open-closed principle. Are all forms in your app going to follow this exact structure?

    Also, I wouldn't use explicit constants for errors, use a defined type such as $errors[$key] = FORM_REQUIRED.

    Oh, and I wouldn't iterate through $_POST, because you might find yourself checking form vars that arn't even used, say if the client edited the form.

    If you want to validate a form var according to a application-wide rule (a rule that is enforced in other parts of the application) you will want to define that rule in one central place, so you don't have to copy paste your regex in every form that deals with an email addr. E.g. preg_match( EMAIL_REGEX )


Bookmarks

Posting Permissions

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