CODEWITHSUNDEEP
phpfilefopen
sameold
sameold

Reputation: 19242

Does it make sense to close a file that couldn't be opened

I'm reviewing code that was written by another developer. The code tries to open a file, and sets $canOpen based on success/failure. 

$fh = @fopen('files.php', 'a+');        
if (!$fh){
   fclose($fh);
   $canOpen = false;
} else {
   $canOpen = true; 
}

What I find strange is that it also tries to close the file but only when the open fails if (!$fh). Does this make sense? Shouldn't the close be in the else statement when the file was successfully opened?

Upvotes: 2

Views: 234

Answers (4)

Spikey21
Spikey21

Reputation: 439

No it doesn't make sense, if you cannot open the file you won't need to close it eigther. At this point the filepointer is always open because if the file doesn't exists it will create a file.

Upvotes: 1

Tim G
Tim G

Reputation: 1822

if you put a var_dump( $fh ) in the true block of your if, you'll find that it's not a resource handle.

The php manual states that fclose takes a resource. Therefore fclose can't be called on a file that can't be opened.

<?php
$fh = @fopen('files.php', 'a+');

if (!$fh){
    var_dump( $fh );
    fclose($fh); // this will cause an error
    $canOpen = false;
} else {
    $canOpen = true; 
}

Upvotes: 1

cdeszaq
cdeszaq

Reputation: 31280

The only reason one would close an unopened file is if it is for resiliency purposes. For example, always closing the file in the finally block of a try...catch.

In your case, it looks like a coding error.

Upvotes: 1

DaveRandom
DaveRandom

Reputation: 88647

No, it makes no sense.

If !$fh is met as a condition, it means the $fh contains boolean FALSE (or possibly, in the event of some weird internal PHP error, NULL). So what you are effectively doing is:

fclose(FALSE);

...which is pointless and will result in an error.

And, if all you're trying to do is populate the $canOpen variable and not do anything useful with the file handle, wouldn't a combination of is_file(), is_readable() and is_writable() suffice?

Upvotes: 1

Related Questions