Page 1 of 2 12 Last
  • Jump to page:
    #1
  1. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Dec 2011
    Posts
    39
    Rep Power
    3

    Simple while loop help DOH!


    Hi all

    I been having some issues with a function I am coding for a eCommerce site, and this custom task needed is nearly done bar sorting out if they ordered more than one.

    Basically you order a product and the transaction email pulls a license key from a table and posts it below the product title in the email then deletes that key from the DB

    But I need to check that if the quantity is more than one post an additional key for every extra one. Easier said than done it seems...

    Hope you guys can help, its one of those where I know it can be done but cant find out how too

    PHP Code:
    //GET PRODUCT DETAILS
        
    $result_product_id mysql_query("SELECT * FROM "WPSC_TABLE_CART_CONTENTS ." WHERE purchaseid = '".$order_id."'");

        while(
    $row_product_id mysql_fetch_array($result_product_id))
        {
            
    $prodid $row_product_id['prodid'];
            
    $product_name $row_product_id['name'];
            
    $product_quantity $row_product_id['quantity'];
            
    $product_price $row_product_id['price'];
            
    $product_price_quantity $product_price*$product_quantity;
            
            
    //GET PRODUCT LICENSE KEYS////////////////////////////////////////////////////////////
                                
                
    $result_product_license mysql_query("SELECT * FROM wp_product_keys WHERE productid = '"$prodid ."'");
                    
                while(
    $row_product_license mysql_fetch_array($result_product_license))
                {
                    
    $license $row_product_license['licenseid'];
                }
                            
                
                
    $licensecodefull "Product license key: "$license;
                
    $licensecodeempty "Please contact <a href='mailto:#######?Subject=License Key Request - "$product_name ."'>########</a> for your product license key";
                
                if (
    $license == ""){
                    
                    
    $licensecode $licensecodeempty;
                    
                }else{
                    
                    
    $licensecode $licensecodefull;
                }
                    
            
    //GET PRODUCT LICENSE KEYS//////////////////////////////////////////////////////////// 
    then echos into html

    PHP Code:

    //Just HTML. Customize it your way                
                    
    $product_list_custom .= '<tbody bgcolor="#ffffff"><tr><td align="left" valign="middle" style="padding:3px 9px; border-bottom:1px solid #eeeeee;width: 31px;"><img src="'.$thumbnail_url.'"></td><td align="left" valign="middle" style="padding:3px 9px; border-bottom:1px solid #eeeeee;"><strong>'.$product_name.'</strong><br />'$licensecode .'</td><td align="center" valign="middle" style="padding:3px 9px; border-bottom:1px solid #eeeeee;">'.$product_quantity.'</td><td align="right" valign="middle" style="padding:3px 9px; border-bottom:1px solid #eeeeee;"><span>£'.$product_price_quantity.'</span></td></tr></tbody>';
                    
                    
    $result_product_license_delete mysql_query("DELETE FROM wp_product_keys WHERE licenseid = '"$license ."' AND productid = '"$prodid ."'");
                    
                    
                    
                    }
                
            }
            
        } 
  2. #2
  3. No Profile Picture
    Contributing User
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Dec 2004
    Posts
    2,967
    Rep Power
    374
    you inner while loop needs to have an array i.e.

    not this: $license = $row_product_license['licenseid'];
    but thus: $license[] = $row_product_license['licenseid'];

    otherwise if you are only expecting ONE licence then dont loop its pointless.
  4. #3
  5. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Dec 2011
    Posts
    39
    Rep Power
    3
    Hi Paul

    Thanks for response, so your saying I need to have the LicenseID fed into an array so I can output numerous licenses

    I added this to a While loop as I thought it was needed as the other values were looped (not the best developer in town lol)

    How would I do this to check against the quantity amount?
  6. #4
  7. Lord of the Dance
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Oct 2003
    Posts
    3,575
    Rep Power
    1906
    use LIMIT in your SQL
    something like this:
    Code:
    SELECT * FROM wp_product_keys WHERE productid = '$prodid' LIMIT $ordered_quantity
    Below are some points that can be an improvement to your code:
    - Combine the product and license query by using JOIN instead.
    - Prevent possible SQL injections. (maybe you do have this as I cant see how you receive the user input)
  8. #5
  9. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,957
    Rep Power
    1045
    Originally Posted by silents11
    I added this to a While loop as I thought it was needed as the other values were looped (not the best developer in town lol)
    When you only want to fetch one row, then you only need one mysql_fetch_array (or whatever you use).

    This is somehow a common misunderstanding. People get so used to the "while (... fetch ...) {...}" that they'll use it everywhere, even when it's completely unnecessary.

    And an addition to MrFujin's list:

    Always specify the columns explicitly, don't use "SELECT *". This "fetch all" pattern is bad practice, because it retrieves more data than you actually need and might even fetch internal columns that you certainly don't want to output (like credit card numbers or whatever).

    No offense, but I'm not sure if it's a good idea to work on eCommerce code when you don't seem to have much PHP experience. There's a lot you can do wrong, which will lead to massive security holes.

    Comments on this post

    • ptr2void agrees
    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. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Dec 2011
    Posts
    39
    Rep Power
    3
    Originally Posted by Jacques1
    When you only want to fetch one row, then you only need one mysql_fetch_array (or whatever you use).

    This is somehow a common misunderstanding. People get so used to the "while (... fetch ...) {...}" that they'll use it everywhere, even when it's completely unnecessary.

    And an addition to MrFujin's list:

    Always specify the columns explicitly, don't use "SELECT *". This "fetch all" pattern is bad practice, because it retrieves more data than you actually need and might even fetch internal columns that you certainly don't want to output (like credit card numbers or whatever).

    No offense, but I'm not sure if it's a good idea to work on eCommerce code when you don't seem to have much PHP experience. There's a lot you can do wrong, which will lead to massive security holes.

    Thank you for your input and no offence taken at the end of the day I am learning and you guys know more than me that is why I am here.

    In regards to security holes for ecommerce the license keys are within a separate table from other details and they are only being outputted to a html email that is all so I dont feel it will create and security holes. As you said I would not dream of touching any of the ecommerce "internals".

    I have taken the array out of a while loop and tried the LIMIT but I dont see how this will count what the quantity is and output the amount of keys referenced to the quantity
    Last edited by silents11; September 14th, 2012 at 04:50 AM.
  12. #7
  13. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,957
    Rep Power
    1045
    Originally Posted by silents11
    In regards to security holes for ecommerce the license keys are within a separate table from other details and they are only being outputted to a html email that is all so I dont feel it will create and security holes.
    Well, unfortunately it's not that simple. Security problems are likely to affect the whole page, not just your specific section. For example, if your queries are vulnerable to SQL injections, an attacker might be able to rewrite the query and fetch critical data from a completely different table.

    So you should at least read up on current best practices and security precautions before you start programming. For example, every value you insert into a query or HTML string must be escaped carefully with the right method (mostly mysql_real_escape_string and htmlentities). It would be better to drop the obsolete "mysql_" functions completely and switch to a modern library which supports prepared statements, but I guess that's hard to integrate into Wordpress.

    And if you know anybody with solid (and up to date) knowledge about PHP, ask him to review your final code. That's not really something for a forum.



    Originally Posted by silents11
    I have taken the array out of a while loop and tried the LIMIT but I dont see how this will count what the quantity is and output the amount of keys referenced to the quantity
    The "LIMIT" clause by MrFujin will limit the number of selected licences to the quantity. You can then output those licences and afterwards check if there are licences missing.

    So you do in fact need a "while" loop, since you have multiple rows. The licences must either be saved in an array (like paulh1983 suggested) or appended to an output string. In your original code you just kept overwriting the same variable, so you ended up with only one license (namely the last one from the query).

    PHP Code:
    $licenses_query mysql_query('
        SELECT
            `licenseid`
        FROM
            `wp_product_keys`
        WHERE
            `productid` = "'
    mysql_real_escape_string($prodid) . '"
        LIMIT
            ' 
    . (int) $product_quantity '
    '
    );

    $output '';
    if (
    mysql_num_rows($licenses_query) == $product_quantity) {        // are there enough licences?
        
    $output 'Your product keys:';
        while(
    $license mysql_fetch_assoc($licenses_query)) { 
            
    $output .= '<br />' $license['licenseid'];
    } else
        
    $output 'You do not have enough licences ...'
    (completely untested!)
    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".
  14. #8
  15. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Dec 2011
    Posts
    39
    Rep Power
    3
    Thank you Jacques1 much appreciate your advice and will take them on board!

    I have tested the code and tried to understand how you have done it...

    heres what I have updated with my values. It gives a mysql_num_rows(): supplied argument is not a valid MySQL result resource warning at the if statement line. and the result out puts the please contact.... line

    is it because of the == $product_quantity

    PHP Code:
    $licenses_query mysql_query("SELECT 'licenseid' FROM 'wp_product_keys' WHERE 'productid' = '"mysql_real_escape_string($prodid) . "' LIMIT '" . (int) $product_quantity "'"); 
                
                
    $output ""
                 
                if (
    mysql_num_rows($licenses_query) == $product_quantity) {        // are there enough licences? 
                
    $output "Your Product license key:";
                 
                while(
    $license mysql_fetch_assoc($licenses_query)) 
                    {
                         
    $output .= "<br />" $license['licenseid'];
                    } 
                }else{                         
                    
    $output "Please contact <a href='###?Subject=License Key Request - "$product_name ."'>###</a> for your product license key"
                } 
  16. #9
  17. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,957
    Rep Power
    1045
    Why did you change the query code? Since you replaced all backticks `` (for identifiers) with single quotes '' (for values), you'll get a syntax error. You now have values instead of column and table names.

    Secondly, there's a reason why I put the query on multiple lines with indentation. This makes the query much more readable and less prone to errors.
    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 Newbie (0 - 499 posts)

    Join Date
    Dec 2011
    Posts
    39
    Rep Power
    3
    Originally Posted by Jacques1
    Why did you change the query code? Since you replaced all backticks `` (for identifiers) with single quotes '' (for values), you'll get a syntax error. You now have values instead of column and table names.

    Secondly, there's a reason why I put the query on multiple lines with indentation. This makes the query much more readable and less prone to errors.
    Please accept my apologies I changed the back ticks as I thought it was how they were copied over as sometimes copying code makes the '' into ``. But I never knew there was a difference so thank you for that totally understand the code now

    plus I have whacked in the code with a few extra } and works perfectly! thank you for your help it is much appreciated.
  20. #11
  21. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Dec 2011
    Posts
    39
    Rep Power
    3
    Hi all

    am back again

    After the success with the code I thought I'd be able to do this simple task but cant find a solution.

    Basically want to delete the keys that are in the loop from the db once they have been outputted.

    I found a solution to delete the values that have only one quantity but the one that gives 2 doesn't delete and am also worried I am not doing it securely.
  22. #12
  23. No Profile Picture
    Contributing User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Dec 2011
    Posts
    39
    Rep Power
    3
    I have got it to work by adding the DELETE within the while and using the $license['licenseid']

    is this secure?

    PHP Code:
    while($license mysql_fetch_assoc($licenses_query)) { 
            
    $output .= $license['licenseid'] . "<br />" 
            
            
    $licenses_query_delete mysql_query("DELETE FROM wp_product_keys WHERE licenseid = '"$license['licenseid'] ."' AND productid = '"$prodid ."'"); 
            } 
  24. #13
  25. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,957
    Rep Power
    1045
    No, that's not secure, because again you didn't escape the values before inserting them into the query string.

    Never insert a raw value into anything that's evaluated (like an SQL query or HTML markup or a console command). If you do, an attacker will be able to manipulate the query/markup/command and do all kind of bad things like logging in as the administrator, stealing credit card numbers, destroying data and whatnot (depending on how well the system is configured). Ask Sony how bad an SQL injection can get.

    So whenever this situation occurs, you must be extremely careful. Every value must be escaped according to the specific context (as I already said above). The ideal solution would be to completely separate code and values, but unfortunately, that's not always possible.

    Again: Never insert unescaped values into code. Always use mysql_real_escape_string() or whatever is the right method for the current context. I must admit that I did the same mistake by not escaping $license['licenseid'] while constructing the $output string.

    Apart from that, I wouldn't delete the keys one by one. It's probably better to first collect all keys in an array and then send to single query to delete them all at once:

    PHP Code:
    $licenses_query mysql_query('
        SELECT
            `licenseid`
        FROM 
            `wp_product_keys`
        WHERE
            `productid` = "'
    mysql_real_escape_string($prodid) . '"
        LIMIT
            ' 
    . (int) $product_quantity '
    '
    );

    $keys = array();
    $output '';
    while(
    $license mysql_fetch_assoc($licenses_query))
        
    $keys[] = $license['licenseid'];
    if (
    count($keys) == $product_quantity) {
        
    $output 'Your Product license keys:<br />' implode('<br />'array_map('htmlentities'$keys));         // note the htmlentities
        // delete keys
        
    mysql_query('
            DELETE
            FROM
                `wp_product_keys`
            WHERE
                `productid` = "' 
    mysql_real_escape_string($prodid) . '"
                AND `licenseid` IN (' 
    implode(','array_map('mysql_real_escape_string'$keys)) . ')                -- note the mysql_real_escape_string
        '
    );
    }
    else
        
    $output 'Not enough licences ...';        // don't forget the escaping 
    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".
  26. #14
  27. Lord of the Dance
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Oct 2003
    Posts
    3,575
    Rep Power
    1906
    Before you delete the products keys, I really hope you have the products keys stored in another place, at least in a backup or similar.

    What if something unexpected happens and the user didn't get the keys?

    Comments on this post

    • Jacques1 agrees
  28. #15
  29. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,957
    Rep Power
    1045
    Originally Posted by MrFujin
    Before you delete the products keys, I really hope you have the products keys stored in another place, at least in a backup or similar.
    Yeah. You should actually use a kind of "deleted" flag instead of actually deleting the keys. There are many situation where you might have to get void keys.
    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".
Page 1 of 2 12 Last
  • Jump to page:

IMN logo majestic logo threadwatch logo seochat tools logo