Page 1 of 1

Peer review for a file_get_contents function

Posted: Fri Jan 27, 2006 9:32 am
by Roja
I'd deeply appreciate some peer review for this function.

I'm fairly certain that this meets most common expectations for a function.. no globals, distinct inputs, clear returns, etc.

I have the nagging feeling that there is something subtly non-ideal.

Feedback is appreciated.

Code: Select all

function file_get_contents($filename, $use_include_path = FALSE)
{
    $data = NULL;
    $file = @fopen($filename, "rb", $use_include_path);
    if ($file)
    {
        while (!feof($file))
        {
            $data .= fread($file, 1024);
        }

        fclose($file);
        return $data;
    }
    else
    {
        return FALSE;
    }
}

Posted: Fri Jan 27, 2006 9:42 am
by feyd
I think there was something I read a while ago about feof() under certain circumstances not returning false. I'm also not entirely sure it works well for remote files...

Posted: Fri Jan 27, 2006 9:50 am
by Jenk
Would the following not be better? (genuine question!)

Code: Select all

<?php

function file_get_contents($path, $use_include_path = FALSE)
{
    $file = @file($path, $use_include_path);

    if ($file !== FALSE) {
        $filestring = implode("\r\n", $path);
    } else {
        return FALSE;
    }

    return $filestring;
}
?>

Posted: Fri Jan 27, 2006 9:54 am
by feyd
lose the \r\n (leave just an empty string) .. it may still have issues, but they would be more internal to php's core than not.

Posted: Fri Jan 27, 2006 4:05 pm
by Christopher
Maybe it's just me, but I am not wild about using @ to supress messages. I won't go so far as to call it evil as other do. I would prefer to code it something like this:

Code: Select all

function file_get_contents($filename, $use_include_path = FALSE) {
    $data = FALSE;
    if (file_exists($filename) && is_file($filename) && is_readable($filename)) {
        $file = fopen($filename, "rb", $use_include_path);
        if ($file) {
            $data = fread($file, filesize($filename));
            fclose($file);
        }
    }
    return $data;
}
This code was not tested. Also I believe PHP caches the file descriptors so each call does not have to reread the file.

Posted: Fri Jan 27, 2006 11:55 pm
by josh
What is the point of reading in on KB at a time, why not just leave the option length parameter blank and read in the file all at once.

edit: ok yeah the documentation explained it, but why check for eof when fread already does that for you?

Posted: Sat Jan 28, 2006 1:02 am
by Christopher
jshpro2 wrote:edit: ok yeah the documentation explained it, but why check for eof when fread already does that for you?
I thought that you only read chunks of you were reading from a stream.

Posted: Sat Jan 28, 2006 2:22 am
by josh
Yeah, I'm guessing he wants his file_get_contents() to work with any kind of wrapper just like the 'real' version of the function