Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:55709
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.214.42 as permitted sender)
MIME-Version: 1.0
Sender: ekneuss@gmail.com
In-Reply-To: <4E8DC9B4.7040809@uw.no>
References: <4E7C764C.3000808@uw.no>
	<4E8DC9B4.7040809@uw.no>
Date: Thu, 6 Oct 2011 18:03:03 +0200
Message-ID: <CACQesk4hh9FxO=L3bERu5-=+vWCqGP28X5Gr6UXczQRN9C3RXA@mail.gmail.com>
To: "Daniel K." <dk@uw.no>
Cc: internals@lists.php.net
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] [PATCH] Fix for bug #55754
From: colder@php.net (Etienne Kneuss)

Hi,

On Thu, Oct 6, 2011 at 17:31, Daniel K. <dk@uw.no> wrote:
> On 09/23/2011 02:06 PM, I wrote:
>> When a built-in function is defined with ZEND_SEND_PREFER_REF, PHP will
>> issue a strict warning if you use an assignment expression as the parame=
ter.
>>
>> As an example, current() show this behaviour.
>>
>> <?php
>> current($foo =3D array("bar"));
>> ?>
>>
>> Presents you with:
>>
>> PHP Strict Standards: =C2=A0Only variables should be passed by reference=
 in
>> %s on line %d
>
> A patch was appended, discussed, and improved, and I have uploaded a
> test-case, as well as a minimal patch that fixes the problem (attached)
> to the original bug-report.
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0https://bugs.php.net/bug.php?id=3D55754
>
> The patch still applies to trunk, and I think it should be applied to
> the 5.3 and 5.4 branches as well.

The new patch seems to be indeed better. My only concern is whether it
covers all those cases now. Did you try passing all possible kinds of
value expressions ? Or is it guaranteed to cover all cases by design?
If not, we should probably add more of those cases in the tests.

Other than that, this looks fine.

>
> What to do next?
>
> The feedback has been constructive, but I feel it has stagnated a bit.
> Of course, you may all be busy with other interesting stuff, but is
> there anything else you require from me to get this rolling?
>
> Should I explain better why I think it's an issue, and why the patch is
> needed?
>
> An indication as to if anyone is reviewing the proposed patch, and
> considering applying it, or telling me that this is the completely wrong
> approach to solve the problem and dropping a few hints would be most
> appreciated, but any feedback is welcome.
>
>
> Daniel K.
>



--=20
Etienne Kneuss
http://www.colder.ch