Page 1 of 1
why won't this simple function work as expected ???
Posted: Wed Apr 30, 2008 4:10 pm
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
which returns "no"
Any ideas why ?
Re: why won't this simple function work as expected ???
Posted: Wed Apr 30, 2008 4:17 pm
by VladSun
Scope issue - you can't access $types inside the function. Use global scope modifier.
Re: why won't this simple function work as expected ???
Posted: Wed Apr 30, 2008 4:21 pm
by scheinarts
ahh! thanks!
Re: why won't this simple function work as expected ???
Posted: Wed Apr 30, 2008 4:23 pm
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.

Re: why won't this simple function work as expected ???
Posted: Wed Apr 30, 2008 9:28 pm
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";
// ....
Re: why won't this simple function work as expected ???
Posted: Thu May 01, 2008 2:13 am
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

Re: why won't this simple function work as expected ???
Posted: Thu May 01, 2008 3:10 am
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.
Re: why won't this simple function work as expected ???
Posted: Mon May 05, 2008 10:56 am
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?
Re: why won't this simple function work as expected ???
Posted: Mon May 05, 2008 1:22 pm
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

Re: why won't this simple function work as expected ???
Posted: Mon May 05, 2008 11:30 pm
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?
Re: why won't this simple function work as expected ???
Posted: Tue May 06, 2008 4:25 am
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).
Re: why won't this simple function work as expected ???
Posted: Tue May 06, 2008 12:52 pm
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.