[PHP] Would this work?

[Luke]

Ok, i'ev got this code, and what happens is when you click the download link it wuold go to count.php?id=2 for example.

<?php
include ('config.php');

$id = $_GET['id'];
if (!is_numeric($id))
{
exit;
}
mysql_query ("UPDATE downloads SET total = total + 1 WHERE id = '$id'"); 
$result = mysql_query("SELECT * FROM downloads WHERE id = '$id'"); 
$row = mysql_fetch_object ($result);
header("Location: " . $row->url); 
?>

Would that add a "number times downloaded" to the SQL database and then forward them to the download?

Thanks
Luke

Moved by Invent (Forum Moderator) from Designing & Development: Please post in the correct forum next time, thanks .

[Source]

<?php
include ('config.php');

//Basic Filter for ID (Stops SQL Injections)
$id = mysql_real_escape_string(htmlentities($_GET['id'], ENT_QUOTES));

//Would work but the filter ^ is there just in case
if (!is_numeric($id))
{
    exit;
}

//Update the download count, I do this a longer but easier to edit way
//Run query
$query = mysql_query( "SELECT * FROM `downloads` WHERE `id` = '$id'" );

//Grab the data into an array (can use object if you want)
$data = mysql_fetch_array( $total );

//Add 1 to the total, I did it this long way as here you can now do more complex
//Sums which I have had todo in the past
$totalnew = $data['total'] + 1;

//Update total to the one we just calculated
$update = mysql_query ("UPDATE `downloads` SET `total` = $totalnew WHERE `id` = '$id'"); 

//Run the query (added ` for more compatibilty)
$result = mysql_query("SELECT * FROM `downloads` WHERE `id` = '$id'"); 

//Get the data into a usable object
$row = mysql_fetch_object ($result);

//Head off to the URL from the database
header("Location: " . $row->url); 

?>

You did some things I personally havn't seen before, and they probably were correct. I just tidied it up a bit and did the download count slightly different as I find it easier to work with multiple lines (gives extra compatibility when you want todo more complex sums in the future).

More experienced php programmers will probably tell me how bad that is, sorry if it is - feel free to correct me.

[Protege]

<?php
include( 'config.php' );

function cleanMe( $yay )
{
    $str = mysql_real_escape_string( $yay );
    return( strip_tags( $str ) );
    // add some more cleaning if you want lalala..
}

/*
 You dont really need the function as you only call it once
 ( well only need to, as you could just clean $id )
 Basically what source said, use it if you understand it.
*/

$id = $_GET[ 'id' ]; // i'll keep that

if( is_numeric( $id ) )
{
    if( mysql_query( 'UPDATE `downloads` SET `total` = `total` + 1 WHERE `id` = "' . cleanMe( $id ) . '"' ) )
    {
        if( $mysqlQuery = mysql_query( 'SELECT * FROM `downloads` WHERE `id` = "' . cleanMe( $id ) . '"' ) )
        {
            if( mysql_num_rows( $mysqlQuery ) >= 1 )
            {
                if( $mysqlFetch = mysql_fetch_array( $mysqlQuery ) )
                {
                    // wayy
                    headers( 'Location: ' . $mysqlFetch[ 'url' ] );
                }
                else
                {
                    echo( 'Error sxi no rows' );
                }
            }
            else
            {
                echo( 'Omg no rowzz' );
            }
        }
        else
        {
            echo( 'Like no accezz or errorz in dbazes' );
        }
    }
}
else
{
    // try not to use exit tbh its nasty (unless it rly satisfies you)
    echo( 'Booby error' );
}
?>

Yours will work but theres some changes id make, and you probs get someone like jewball coming and putting something useless in.

Ok, i'ev got this code, and what happens is when you click the download link it wuold go to count.php?id=2 for example.

<?php
include ('config.php');

$id = $_GET['id'];
if (!is_numeric($id))
{
exit;
}
mysql_query ("UPDATE downloads SET total = total + 1 WHERE id = '$id'"); 
$result = mysql_query("SELECT * FROM downloads WHERE id = '$id'"); 
$row = mysql_fetch_object ($result);
header("Location: " . $row->url); 
?>

Would that add a "number times downloaded" to the SQL database and then forward them to the download?

Thanks
Luke

[Source]

Protege's is more complex and if you understand it, use it, but make sure you actually read through and see what he's doing so you can improve aswel. I get fed up with the countless amount of times I see people just copy and use code without knowing what it does.

[Hypertext]

<?php
include( 'config.php' );

$id = $_GET[ 'id' ]; // i'll keep that

if( is_numeric( $id ) )
{
    if( mysql_query( "UPDATE `downloads` SET `total` = `total` + 1 WHERE `id` = '$id'" ) )
    {
        if( $mysqlQuery = mysql_query( "SELECT * FROM `downloads` WHERE `id` = '$id'" ) )
        {
            if( mysql_num_rows( $mysqlQuery ) > 0 )
            {
                if( $mysqlFetch = mysql_fetch_array( $mysqlQuery ) )
                {
                    // wayy
                    header( 'Location: ' . $mysqlFetch[ 'url' ] );
                }
                else
                {
                    echo 'Error sxi no rows';
                }
            }
            else
            {
                echo 'Omg no rowzz';
            }
        }
        else
        {
            echo 'Like no accezz or errorz in dbazes';
        }
    }
}
else
{
    // try not to use exit tbh its nasty (unless it rly satisfies you)
    echo 'Booby error';
}
?>

Just changed a few things, like that function is pointless as you verify that $_GET['id'] is numeric.

I'd suggest making a db wrapper, and instead of all those if's just put it in they're functions. I'd change it to or die(mysql_error()) but dont want to right now.

Oh and I changed echo's to not use parenthesis, again, and corrected the spelling of header().

@Protege, remember about language constructs that they don't need parenthesis, and it is generally considered bad practice.. well thats what a guy on phpfreaks forums told me..



@update: fixed quotes

[Invent]

<?php
include( 'config.php' );

$id = $_GET[ 'id' ]; // i'll keep that

if( is_numeric( $id ) )
{
    if( mysql_query( "UPDATE `downloads` SET `total` = `total` + 1 WHERE `id` = '". $id ."'" ) )
    {
        if( $mysqlQuery = mysql_query( "SELECT * FROM `downloads` WHERE `id` = '". $id ."'" ) )
        {
            if( mysql_num_rows( $mysqlQuery ) > 0 )
            {
                if( $mysqlFetch = mysql_fetch_array( $mysqlQuery ) )
                {
                    // wayy
                    header( 'Location: ' . $mysqlFetch[ 'url' ] );
                }
                else
                {
                    echo( 'Error sxi no rows' );
                }
            }
            else
            {
                echo( 'Omg no rowzz' );
            }
        }
        else
        {
            echo( 'Like no accezz or errorz in dbazes' );
        }
    }
}
else
{
    // try not to use exit tbh its nasty (unless it rly satisfies you)
    echo 'Booby error';
}
?>

Fixed your errors

@Protege, remember about language constructs that they don't need parenthesis, and it is generally considered bad practice.. well thats what a guy on phpfreaks forums told me..

I'm not sure why it would be considered bad practice unless they slow down scripts by a decent amount - which I dont think they do.

[Hypertext]

I'd imagine it would slow scripts down... wouldn't it have to alot memory for it?

@off-topic: loops going downwards are 90% faster than upwards.. i heard it then my friend did benchmarks.. and he changed a 7k line script to that, sped it up immensely.

[Invent]

Which did you fix?
The quotes one? I just fixed them, ty.

Fantastic.

Why are you promoting echo('this');?

Nothing is wrong with using parenthesis' with language constructs in my opinion. I may run a speed test later to actually see how badly they actually do slow down scripts.

[Source]

It doesn't adversly affect the performance of a tiny snipper of code like that, not even over a big script. Charlie just be simple, you always trip yourself up trying to act smart.

[Hypertext]

Yeh, but Protege always uses them. I'm only trying to explain that it's bad practice. :/

How the **** am I tripping myself up? I'm not even trying to act smart.

@edit I'll benchmark this afternoon (central time)