• symcbean (unregistered)

    Please tell me this didn't come from a web page.

  • (cs)

    What's worse is the whole implode('', file($file)) business. What's the point of slurping a file, doing the whole split-each-line-into-an-array-elementj then, and then turn-the-array-into-a-long-string overhead is crazy, when a better (and still incorrect) method would have been to use file_get_contents() , which loads the file into a string without all the transforms.

    Without fixing the general brokenness of that construct, a more robust wrong solution would be:

    if (is_readable($file)) { $data = file_get_contents($file); } else { $fp = yada yada yada.... }

  • MoroS (unregistered)

    Lol... Now that's nice. If the file exists then open it, if not then try to open it non the less, maybe it'll work. :D

    Don't you just love that kind of thinking?

    CAPTCHA: wigwam... true, better hide before that kind of code gets popular...

  • Michael (unregistered) in reply to tux_rocker
    tux_rocker:
    It seems like the author assumed that if the file does not exist on the local filesystem, then it must be a URL. And under certain PHP configurations, fopen() can open remote files if you pass it a URL. file_exists() however only checks for local files.

    I agree that this is the likely cause. Programmer always checks if file exists before opening. Later, he finds that some URLs return false when passed to file_exists(). So he creates the else block, probably assuming that feof() will return true when passed an invalid filehandle. He tests it with a valid URL, it works, he's done.

    The real WTF from a php perspective is not using file_get_contents, or assuming that implode("", file($file)) will somehow be more efficient that reading kilobyte blocks. Probably a case of thinking that if it take fewer lines to write, it takes fewer cycles to execute.

  • + (unregistered)

    The real WTF is checking for the file before opening it; after all someone could have come along and deleted the file between the check and the fopen(), and then where would he be?

  • Barry Price (unregistered)
  • Barry Price (unregistered)

    Wow. I saw exactly this happen many moons ago, working for an ISP in the North-West of England (oh, okay, it was called Supanet).

    A "team leader" had written code identical to this. I fixed it, and emailed her privately to point out the problem.

    In return, I got an email cc'd to the rest of the development team admonishing me for meddling with code I didn't understand...

  • Zemyla (unregistered)

    Relatively unrelated, but now I am going to call all my cron jobs "cronies".

  • (cs) in reply to Zemyla

    The Real WTF is that /var/log/ is on the /var partition and not its own. Nothing like DoS-ing the server by filling up its log space...

    Addendum (2007-01-29 14:18): Edit: I guess it helps to read the whole thing carefully... errors were in the mail queue, not the log files. :-P

  • tgies (unregistered) in reply to shortbread
    shortbread:
    $data = $data . fread($fp, 1024);

    Should have been:

    $data .= fread($fp, 1024)

    but even then that's just evil. Looks like the $data buffer doubles in size + 1024 every iteration and STILL it's an infinite loop.

    captcha: dreadlocks (groovy!)

    What? No. They're the exact same thing.

    tux_rocker:
    It seems like the author assumed that if the file does not exist on the local filesystem, then it must be a URL. And under certain PHP configurations, fopen() can open remote files if you pass it a URL. file_exists() however only checks for local files.

    That's what I thought as well. Reading in 1k blocks like that is the de facto standard for fetching files via HTTP, for whatever reason.

  • Scott (unregistered)

    I consider this a mixture of user WTF AND PHP WTF, if a file pointer is invalid PHP still returns false for feof so if you had a nice loop like that of the article your going to end up in an infinite loop.

    The nicer thing to do would be to return true or trigger a fatal error.

  • Matias (unregistered)

    File not found, ah? We will see about that...

  • coroner (unregistered) in reply to Philihp

    i would write the whole crap as

    $date=(is_readable($file))?implode('',file($file)):'';

    :)

  • (cs)

    Goober: Hey does that car have a trunk? Some guy: No. Goober: Ok, open the trunk and unload everything in there.

  • coroner (unregistered) in reply to coroner

    ahem. sorry for this, had a typo... i meant $data instead of $date

  • Pent (unregistered)

    And that's exactly the reason why I hate languages that [allow you to] ignore or "fix" errors...

  • (cs)

    switch(file_exists($file)){ case true: #open file and read it. case false: #open file and read it. case FILE_NOT_FOUND: #Do nothing... It isn't even explicit at the code.

    I still don't see the problem :)

  • (cs)

    Jesus Christ in a handbag, 70 comments and no one has yet figured out the difference between file_get_contents() and implode('',file())?

    Hint: newlines

    file() will convert a file into a newline-delimited array, which implode will then mush back together. In other words, it's a rather inefficient way of stripping newlines. file_get_contents() is equivalent to implode("\n",file()), not the OP, or you could use str_replace("\n",'',file_get_contents()) to get the more efficient form.

    (I'm just glad php didn't try to pass off file_get_contents() and fileputcontents() as a good idea, although who thought leaving file_put_contents() off until php5 was a good idea should be put in stocks.)

  • MoroS (unregistered) in reply to +
    +:
    The real WTF is checking for the file before opening it; after all someone could have come along and deleted the file between the check and the fopen(), and then where would he be?

    Sadly, what you said is a WTF. In PHP the file_exists function needs only a filename, not a file handle. Besides, who in Gods name would open a file not knowing if it's there?

    CAPTCHA: muhahaha... yes, I'm evil. :>

  • Peter (unregistered)

    I was thinking for a moment it was something to do with the file mode used -- in this case r... it would almost make sense if something like a+ was used, notwithstanding the trying to read from the file.. The result of this, for the PHP-unenlightened is "attempt to open the file and create it if it doesn't exist"... which you could understand, as opposed to r which just means "open for reading".

    captcha: sanitarium. saniittuurrrioommm, saniittruu... BRAIN!

  • Michael (unregistered) in reply to foxyshadis
    foxyshadis:
    Jesus Christ in a handbag, 70 comments and no one has yet figured out the difference between file_get_contents() and implode('',file())?

    Hint: newlines

    file() will convert a file into a newline-delimited array, which implode will then mush back together. In other words, it's a rather inefficient way of stripping newlines. file_get_contents() is equivalent to implode("\n",file()), not the OP, or you could use str_replace("\n",'',file_get_contents()) to get the more efficient form.

    (I'm just glad php didn't try to pass off file_get_contents() and fileputcontents() as a good idea, although who thought leaving file_put_contents() off until php5 was a good idea should be put in stocks.)

    71 comments later and we get someone who starts trashing other people while clearly demonstrating his own failed understanding of the PHP functions in question. Here is the important part of the manual for file():

    Identical to file_get_contents(), except that file() returns the file in an array. Each element of the array corresponds to a line in the file, with the newline still attached.
    So as it turns out, there is no difference between implode("", file($filename)) and file_get_contents($filename). Sorry, you lose, thanks for playing.
  • RockinJack (unregistered)

    Hahaha, this is the best:

    $data = implode("", file($file));

    what data do you get after you 'implode' a file?

    Hilarious.

  • Michael (unregistered) in reply to coroner
    coroner:
    i would write the whole crap as

    $date=(is_readable($file))?implode('',file($file)):'';

    :)

    Carefull there, it might evaluate both sides before decideing which one should be returned.

    (Admittedly i haven't done any php for a year and don't know it's behaviour. Just don't try it in VB :) (no not the beer, the scary story!))

    CAPTCHA: tastey ? where is the chicken?

  • (cs) in reply to snoofle
    snoofle:
    Ah but for a simple "!"...

    This required two simple unit tests to validate. sighs

    Why is this a problem with the "!"

    Looks pretty explicit to me: If file exists read file else open file for read.

  • Baston (unregistered)

    Once again, the usefull boolean True/False/File_Not_Found should have been the key :o)

  • (cs) in reply to Michael
    Michael:
    coroner:
    i would write the whole crap as

    $date=(is_readable($file))?implode('',file($file)):'';

    :)

    Carefull there, it might evaluate both sides before decideing which one should be returned.

    (Admittedly i haven't done any php for a year and don't know it's behaviour. Just don't try it in VB :) (no not the beer, the scary story!))

    CAPTCHA: tastey ? where is the chicken?

    You are aware that the part of the statement to the right of the = is the ternary operator (AKA in-line if), right? Of course it doesn't evaluate the middle or right parts of the ternary until figuring out which of them it needs!

  • ex-php coder (unregistered)

    Most likely the person who wrote this tought that file_exists function was is_file_open or so.

    That would explane why the file was first opened in else statement. This is someting I would call brainfart.

    Ps. Yes i know that is_file_open function does not exist in PHP.

  • bob (unregistered) in reply to powerlord
    powerlord:
    Michael:
    coroner:
    i would write the whole crap as

    $date=(is_readable($file))?implode('',file($file)):'';

    :)

    Carefull there, it might evaluate both sides before decideing which one should be returned.

    (Admittedly i haven't done any php for a year and don't know it's behaviour. Just don't try it in VB :) (no not the beer, the scary story!))

    CAPTCHA: tastey ? where is the chicken?

    You are aware that the part of the statement to the right of the = is the ternary operator (AKA in-line if), right? Of course it doesn't evaluate the middle or right parts of the ternary until figuring out which of them it needs!

    My guess is that he was referring to the use of $date, which in VB returns the current date.

  • Benjamin Smith (unregistered) in reply to SomeCoder
    SomeCoder:

    I know a PHP programmer who will write all his cron jobs/tasks/whatever in PHP. The jobs are usually WAY more suitable for Perl but he insists on using the command line PHP for it.

    The right tool for the right job does not mean that you should use a web programming language for scheduled/cron tasks....

    Captcha: cognac.... a little early but if you insist!

    Awww, come on. How is the PHP interpreter any heavier or painful than the PERL interpreter?

    Truth is, it isn't. Either tool is quite sufficient for the job.

    I guess what I should be asking is: do the scripts do what's needed? If the answer is 'yes', then your friend is on the money.

    It's most likely that you're out in left field...

  • coroner (unregistered) in reply to foxyshadis

    Yes Sir, but did you recognize the file_get_contents() isn't available on [ php -lt 5 ] ?

    Besides, all of your suggestions are not really newline safe, the only way to get a file with mixed content into a variable without loss of any codes is setting the ob_handler and eval('?>'.ob_get_contents()); the result of readfile()....

    as this counts for all file with unknown content, i assume it isn't a good idea to process files with generally unknown content ...

  • coroner (unregistered) in reply to bob
    bob:
    powerlord:
    Michael:
    coroner:
    i would write the whole crap as

    $date=(is_readable($file))?implode('',file($file)):'';

    :)

    Carefull there, it might evaluate both sides before decideing which one should be returned.

    (Admittedly i haven't done any php for a year and don't know it's behaviour. Just don't try it in VB :) (no not the beer, the scary story!))

    CAPTCHA: tastey ? where is the chicken?

    You are aware that the part of the statement to the right of the = is the ternary operator (AKA in-line if), right? Of course it doesn't evaluate the middle or right parts of the ternary until figuring out which of them it needs!

    My guess is that he was referring to the use of $date, which in VB returns the current date.

    ... sorry for this, it was a typo and should read $data instead of $date

  • PC Paul (unregistered) in reply to Barry Price
    Barry Price:
    Wow. I saw exactly this happen many moons ago, working for an ISP in the North-West of England (oh, okay, it was called Supanet).

    A "team leader" had written code identical to this. I fixed it, and emailed her privately to point out the problem.

    In return, I got an email cc'd to the rest of the development team admonishing me for meddling with code I didn't understand...

    And your response...?

    Or is that connected to the fact that your employer was called SupaNet?

  • Daniel15 (unregistered) in reply to coroner
    coroner:
    i would write the whole crap as

    $date=(is_readable($file))?implode('',file($file)):'';

    :)

    $data = (is_readable($file) ? file_get_contents($file) : ''; is better, as it uses file_get_contents :D

    Actually, if the file doesn't exist, shouldn't it stop the script or something?

  • (cs) in reply to Daniel15
    Daniel15:
    coroner:
    i would write the whole crap as

    $date=(is_readable($file))?implode('',file($file)):'';

    :)

    $data = (is_readable($file) ? file_get_contents($file) : ''; is better, as it uses file_get_contents :D

    Actually, if the file doesn't exist, shouldn't it stop the script or something?

    Oops, missed a bracket $data = (is_readable($file)) ? file_get_contents($file) : '';

  • Martijn (unregistered) in reply to Daniel15

    So, the WTF is basically due to a double negative "if" statement; !feof() = Not no more data.

    if there existed a fmore() (Or whatever name. Except fneof() ;) ) instead of feof(), this problem wouldn't have existed, since the logical result of passing an invalid pointer to the function "Is there more data in this file?" would have returned FALSE, albeit this time without the need for "!". The loop would have stopped on first iteration.

    I know, I know, the WTF is that the dumbass tried to read the file whilst knowing it doesn't exists, but still... think about it ;)

  • (cs) in reply to Martijn

    Love the logic!

    1. If $file exists, read $file.
    2. Otherwise (i.e. if $file doesn't exist), read $file.

    Maybe that guy can go to work on the vice-President's next Iraq policy...

  • (cs) in reply to coroner
    coroner:
    Yes Sir, but did you recognize the file_get_contents() isn't available on [ php -lt 5 ] ?

    Besides, all of your suggestions are not really newline safe, the only way to get a file with mixed content into a variable without loss of any codes is setting the ob_handler and eval('?>'.ob_get_contents()); the result of readfile()....

    as this counts for all file with unknown content, i assume it isn't a good idea to process files with generally unknown content ...

    file_get_contents() showed up in 4.3. file_put_contents() was the one that came around in 5.0.

    You have one fucked up was of reading files - if you want newline normalization, there are easier methods that don't slurp memory like a glutton. (Due to php's nonvolatile strings, the fread method can also slow to a crawl after a few megs though, with a measly 1K buffer.)

    And yes, I guess I failed hard with the file() thing, but I was still sure that happened. Maybe I'm carrying over some bad 3.x experiences or something.

  • PHP must be evil (unregistered) in reply to MoroS
    MoroS:
    +:
    The real WTF is checking for the file before opening it; after all someone could have come along and deleted the file between the check and the fopen(), and then where would he be?

    Sadly, what you said is a WTF. In PHP the file_exists function needs only a filename, not a file handle. Besides, who in Gods name would open a file not knowing if it's there?

    CAPTCHA: muhahaha... yes, I'm evil. :>

    I hope you're being sarcastic. Opening a file should include checking for its existence as an atomic operation, for the reasons the original guy stated. Or is PHP (I don't know the language) really just that bug-ridden by design?

  • bad boss (unregistered)

    Not understanding it.

    How did the errors from an infinite loop get into the mail stream. I presume that a webserver process hit a timeout and killed off the job, thereby killing any open files.

    Or am I wrong in that the files got closed gracefully and delivered to the mail stream.

  • darwin (unregistered) in reply to MoroS
    MoroS:
    +:
    The real WTF is checking for the file before opening it; after all someone could have come along and deleted the file between the check and the fopen(), and then where would he be?

    Sadly, what you said is a WTF. In PHP the file_exists function needs only a filename, not a file handle. Besides, who in Gods name would open a file not knowing if it's there?

    No, he was right. It is a mistake to ever check for the existence of any resource before trying to access it. Even if it exists, there is no reason to expect that it will STILL exist a millisecond (or a nanosecond) later when you try to access it.

    You should simply try to access it and handle the error (or catch the exception) if it does not exist.

    This thinking that it's better to check first is rooted in a single-user, single-processing mentality that is not just incompatible with but downright dangerous on modern multi-user, multi-processing, multi-threaded systems. An even worse example of the same thing is trying to replicate all the OS's tests in your own code to check and see whether a file is going to be readable by you, because in addition to the problems already mentioned, even if you succeed you can only end up with a horrible violation of the DRY principle.

    And what on earth does that bit about filename vs. file handle have to do with any of this?

  • /Arthur (unregistered)

    This still does not explain why the /var was full

  • N-state (unregistered) in reply to Noone
    Noone:
    The biggest WTF is that he sent PHP to do a job of a PERL, everyone knows that you shouldn't send a boy for a job of a man.

    (Yes I hate PHP, for valid reasons too)

    Then I suppose sending Python would be overkill - being a god after all, carrying forward your metaphor.

    And I agree - I hate PHP too.

  • EchoBinary (unregistered) in reply to Noone

    Language is secondary to sound logic.

  • blademonkey (unregistered) in reply to EchoBinary

    first of all, that makes no sense. {= )

  • EchoBinary (unregistered) in reply to blademonkey

    WTF?bath

  • tobozo (unregistered) in reply to Noone

    if the guy had done exactly the same wtf in perl, it just would'nt be human readable and not even laughable

    (Yes I hate Perl, for valid reasons too)

  • Mark (unregistered)

    WTF!

  • Emu (unregistered)

    So if file_exists doesn't work for URLs, what does?

    (Don't say just open it and catch errors... this is for an image library script I'm working on, and I want to check if a remotely hosted image exists before 302ing to it)

    fopen works for URLs, but what would that actually do? load it into memory ready for reading, or just get a handle?

  • GrouchyAdmin (unregistered) in reply to Emu
    Emu:
    fopen works for URLs, but what would that actually do? load it into memory ready for reading, or just get a handle?

    fopen would return the contents of the file if URL handling glue was in it. A better method would be to use cURL, or just write your own socket/get/etc code. I usually do the latter, because I enjoy stabbing myself in the leg.

Leave a comment on “Well, Open it Anyway”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article