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
Roja
Tutorials Group
Posts: 2692 Joined: Sun Jan 04, 2004 10:30 pm
Post
by Roja » Fri Jan 27, 2006 9:32 am
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;
}
}
feyd
Neighborhood Spidermoddy
Posts: 31559 Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA
Post
by feyd » Fri Jan 27, 2006 9:42 am
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...
Jenk
DevNet Master
Posts: 3587 Joined: Mon Sep 19, 2005 6:24 am
Location: London
Post
by Jenk » Fri Jan 27, 2006 9:50 am
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;
}
?>
feyd
Neighborhood Spidermoddy
Posts: 31559 Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA
Post
by feyd » Fri Jan 27, 2006 9:54 am
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.
Christopher
Site Administrator
Posts: 13596 Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US
Post
by Christopher » Fri Jan 27, 2006 4:05 pm
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 » Fri Jan 27, 2006 11:55 pm
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?
Christopher
Site Administrator
Posts: 13596 Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US
Post
by Christopher » Sat Jan 28, 2006 1:02 am
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 » Sat Jan 28, 2006 2:22 am
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