Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:65657
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.214.169 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CALeP-ExAiAbiAPgtw8Dh_U5BpBtNGB=sGqMpOCzeecE3WrbP5g@mail.gmail.com>
References: <CALeP-ExAiAbiAPgtw8Dh_U5BpBtNGB=sGqMpOCzeecE3WrbP5g@mail.gmail.com>
Date: Mon, 4 Feb 2013 18:51:55 -0700
Message-ID: <CAFMT4Nq2PNC-oAsvjk1tZc0MKwogoPAVomF6XsNyzC2KO0Vb4g@mail.gmail.com>
To: Charles Sprayberry <cspray.php@gmail.com>
Cc: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=ISO-8859-1
Subject: Re: [PHP-DEV] SplFileObject::__toString potentially returns false
From: morrison.levi@gmail.com (Levi Morrison)

On Mon, Feb 4, 2013 at 5:56 PM, Charles Sprayberry <cspray.php@gmail.com> wrote:
> There was a previous discussion on the return value of
> SplFileObject::__toString being
> an alias to SplFileObject::current() but that was about the design decision
> why and not
> really suggesting any kind of change or a technical error. In the
> discussion it was
> established that:
>
> (1) Changing this would be a BC break
> (2) It isn't really a bug in the codebase, the docs were just wrong
>
> The current implementation has a bug in that current() can potentially
> return false, if
> false is returned from the __toString() magic method a catchable fatal
> error occurs.
>
> Code ran on 5.4.11 *nix, Apache 2.2
>
> --TEST--
> Ensuring we get appropriate output for SplFileObject::__toString
> --FILE--
> <?php
>
> $file = fopen('file.txt', 'w');
> fwrite($file, 'A');
> fclose($file);
>
> $FileObject = new SplFileObject('file.txt');
> foreach ($FileObject as $line) {
>     echo $FileObject;
> }
>
> echo $FileObject;
> ?>
> --EXPECTF--
> A
>
>
> ---------
> Diff
>
> 002+ Catchable fatal error: Method SplFileObject::__toString() must return
> a string value
> in /path/to/splfileobject_tostring.php on line 12
>
> ---------
>
> I would say that SplFileObject:__toString() being an alias to
> SplFileObject::current()
> makes the use cases for the method very limited and that the current
> implementation is
> mistaken in returning current(). I can think of no use case where
> SplFileObject::__toString()
> should return this value. If need to get the current line you're probably
> iterating and in
> that case should be using foreach() and don't really need to manually take
> care of that.
> Even if there is a use case for manually invoking
> `SplFileObject::current()` you probably
> aren't casting a string in that situation and a more expressive method call
> is better.
>
> (1) Leave the implementation the way it is and simply return a blank string
> when
> current() would return false
>
> (2) Change the implementation to return SplFileInfo::__toString()
>
> (3) Change the implementation to return entire file contents
>
> (4) Return some other unthought of string but remains consistent and logical
>
> I am personally partial to (2) or (3) perhaps in 5.5 but (1) could probably
> happen
> sooner? Thoughts?

I don't really use this particular object, but from a distance I think
something should change.  Returning false seems like a bug or design
flaw.  I can probably be persuaded for any of options 1-3 though I
prefer them in 2, 1, 3 order.

Looking it up in the helly docs
(http://www.php.net/~helly/php/ext/spl/classSplFileObject.html#a79319d744507f828a752981e6f63bf15)
I see that this was always the intended behavior, though I don't know
why.