Peer review for a file_get_contents function

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
Roja
Tutorials Group
Posts: 2692
Joined: Sun Jan 04, 2004 10:30 pm

Peer review for a file_get_contents function

Post 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;
    }
}
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post 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...
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post 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;
}
?>
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post 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.
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post 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.
(#10850)
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Post 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?
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post 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.
(#10850)
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Post 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
Post Reply