why won't this simple function work as expected ???

PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!

Moderator: General Moderators

Post Reply
scheinarts
Forum Commoner
Posts: 52
Joined: Wed Jul 25, 2007 2:37 am

why won't this simple function work as expected ???

Post by scheinarts »

I made this little function to check for the extension type but it does not return true if it finds a match, and I KNOW that the file type I am sending is in the types array

Code: Select all

$type = $_FILES['file']['type'];
 
$types = array("image/gif", "image/jpeg", "image/jpg", "image/png");
 
function checkType($toCheck)
{
    $i = 0;
    $res = "no";
    
    while($i < count($types))
    {
        if ($toCheck == $types[$i])
        {
            $res = "ok";
        }
        $i++;
    }
    return $res;
}
calling it like this

Code: Select all

if (checkType($type) == "ok")
which returns "no"

:banghead: :banghead: :banghead:

Any ideas why ?
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Re: why won't this simple function work as expected ???

Post by VladSun »

Scope issue - you can't access $types inside the function. Use global scope modifier.
There are 10 types of people in this world, those who understand binary and those who don't
scheinarts
Forum Commoner
Posts: 52
Joined: Wed Jul 25, 2007 2:37 am

Re: why won't this simple function work as expected ???

Post by scheinarts »

ahh! thanks!
User avatar
flying_circus
Forum Regular
Posts: 732
Joined: Wed Mar 05, 2008 10:23 pm
Location: Sunriver, OR

Re: why won't this simple function work as expected ???

Post by flying_circus »

The problem is here:

Code: Select all

<?php
 
while($i < count($types))
 
?>
your $types variable is outside of the function scope.

you can add "global $types;" inside of your function, but that is very poor. Pass it to the function as a variable when the function is called, like:

Code: Select all

<?php
 
$type = $_FILES['file']['type'];
 
$types = array("image/gif", "image/jpeg", "image/jpg", "image/png");
 
function checkType($toCheck,$typesCheck)
{
    $i = 0;
    $res = "no";
    
    while($i < count($typesCheck))
    {
        if ($toCheck == $typesCheck[$i])
        {
            $res = "ok";
        }
        $i++;
    }
    return $res;
}
 
 
 
 
# Call method
  if (checkType($type,$types) == "ok") {
    print "Great Sucess!";
  }
?>

EDIT: Wow, you guys are fast. 8O
ggggqqqqihc
Forum Newbie
Posts: 5
Joined: Mon Apr 28, 2008 8:08 pm
Location: China

Re: why won't this simple function work as expected ???

Post by ggggqqqqihc »

change to this:

Code: Select all

 
$type = $_FILES['file']['type'];
 
$types = array("image/gif", "image/jpeg", "image/jpg", "image/png");
 
function checkType($toCheck)
{
    global $types; // use 'global'
    $i = 0;
    $res = "no";
    // ....
 
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: why won't this simple function work as expected ???

Post by Mordred »

So much good advice, and yet so much unhit nails:

1. "yes" and "no" are better represented as true and false. Use ++$i instead of $i++.
2. Rewrite your loop: When you find a match, return or break immediately, there's no need to go on.
3. Remove your loop: Always use built-in functions when they are available. In your case: in_array()
4. Remove your function: $_FILES['file']['type'] comes from the user and should not be trusted. Checking it if it has a "good" value is meaningless.

Hammer of doom mode off :)
User avatar
onion2k
Jedi Mod
Posts: 5263
Joined: Tue Dec 21, 2004 5:03 pm
Location: usrlab.com

Re: why won't this simple function work as expected ???

Post by onion2k »

Don't use global. Pass the array in as a parameter. Otherwise you're going to having big problems if you ever need to reuse the code in another place where $types is called something else, or if you need to check against two different arrays.
User avatar
flying_circus
Forum Regular
Posts: 732
Joined: Wed Mar 05, 2008 10:23 pm
Location: Sunriver, OR

Re: why won't this simple function work as expected ???

Post by flying_circus »

Mordred wrote:Use ++$i instead of $i++.
I'm curious about this statement.

I'm not sure I understand the difference, could you please explain?
User avatar
Apollo
Forum Regular
Posts: 794
Joined: Wed Apr 30, 2008 2:34 am

Re: why won't this simple function work as expected ???

Post by Apollo »

flying_circus wrote:
Mordred wrote:Use ++$i instead of $i++.
I'm curious about this statement.

I'm not sure I understand the difference, could you please explain?
Me too. The only difference is in the return value: the expression ++$i returns the value of $i after increasing it, and $i++ evaluates to the value before increasing $i.
So for example, if $i is 5, then $j = ++$i; would result in $i and $j both being 6, whereas $j = $i++; would result in $j being 5 and $i being 6.

But since the result of the expression is not used here, it shouldn't make any difference at all..?

There are theoretical optimization considerations as why ++var would be faster than var++ (because then it doesn't have to maintain the original value, just the result) but that doesn't apply to PHP. And if this kind of speed improvement is an issue, then certainly PHP isn't suitable for this case in the first place :)
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: why won't this simple function work as expected ???

Post by Mordred »

Yes, ++$i comes from C++ experience and old measurements of PHP code, so it might not be as relevant now. Still, having a good style will not hurt anyone.
The advices were graded though, and this was from the first tier, each one obsoleting the previous. Would you care to comment the other ones?
User avatar
Apollo
Forum Regular
Posts: 794
Joined: Wed Apr 30, 2008 2:34 am

Re: why won't this simple function work as expected ???

Post by Apollo »

I totally agree with true/false being better than "yes"/"no" (or "ok"/"no" <- see scheinarts, this kind of confusion is just one reason :)) and with 2 & 3.

Regarding 4: it's true that $FILES['file']['type'] cannot be trusted, but isn't that what the check is for? Essentially he's forcing the variable to be one of the known values (albeit not in the most efficient way).
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: why won't this simple function work as expected ???

Post by Mordred »

Apollo wrote:Regarding 4: it's true that $FILES['file']['type'] cannot be trusted, but isn't that what the check is for? Essentially he's forcing the variable to be one of the known values (albeit not in the most efficient way).
He's not forcing anything, merely checking. And even if he were forcing it, it wouldn't be correct, as it would have been done based on attacker-controlled input, therefore moot. Actually, this cannot be reliably done (I don't believe in magic, and I doubly don't believe in mimetype magic) so the solution is to not do it at all.
Post Reply