Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:124537
Feedback-ID: id5114917:Fastmail
Date: Sun, 21 Jul 2024 16:11:49 +0100
To: internals@lists.php.net
Subject: =?US-ASCII?Q?Re=3A_=5BPHP-DEV=5D_Request_for_opinions=3A_bug_vs_f?=
 =?US-ASCII?Q?eature_-_change_intokenization_of_yield_from?=
User-Agent: K-9 Mail for Android
In-Reply-To: <86eef5d8-6ffc-45e0-8ed6-6201a414cfb7@bastelstu.be>
References: <66984FD0.5090805@adviesenzo.nl> <AM8P250MB0170FFCA0014FB9EC272DB4FE2AC2@AM8P250MB0170.EURP250.PROD.OUTLOOK.COM> <c9736385-94b7-4064-911f-e5fc1df7e2bd@gmx.de> <AM8P250MB0170B4C59B4313A565CD029AE2AC2@AM8P250MB0170.EURP250.PROD.OUTLOOK.COM> <6699F817.8070806@adviesenzo.nl> <9571bb82-9873-4319-9bd1-0361748335be@bastelstu.be> <669BDB00.70507@adviesenzo.nl> <fa7db246-1577-43b7-a117-c18a73140091@bastelstu.be> <669BE870.2050908@adviesenzo.nl> <b7b47e7a-fc20-4774-bc49-fff271386504@bastelstu.be> <CADK1yXK4bapqAWLZLNk--Fvo87AOv2qbSM1mTn23gU1JSMcL_g@mail.gmail.com> <86eef5d8-6ffc-45e0-8ed6-6201a414cfb7@bastelstu.be>
Message-ID: <2C00EB00-0BA4-40F8-92A5-50B49A639AD6@rwec.co.uk>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/alternative;
 boundary=----CYY9BGP63YI61GBYEIIZ8W6PLCDAQM
Content-Transfer-Encoding: 7bit
From: imsop.php@rwec.co.uk ("Rowan Tommins [IMSoP]")

------CYY9BGP63YI61GBYEIIZ8W6PLCDAQM
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable

On 21 July 2024 12:25:27 BST, "Tim D=C3=BCsterhus" <tim@bastelstu=2Ebe> wro=
te:
>The change has also been explicitly acked by Gina and Christoph before Il=
ija committed it and Bob also participated in the PR without raising concer=
ns, so it's also not an unapproved change=2E The fact that none of them ant=
icipated this side effect, doesn't make it unapproved=2E

I note that Gina commented:=20

> This is kinda cursed=2E=2E=2E but sure why not=2E

And Christoph wrote:

> Let's say that I'm generally not happy with complicating the parser with=
 work-arounds, but this is okay for me=2E

So, hardly a confident consensus that this was the right approach=2E Regar=
dless, we are where we are, and there's general agreement that the current =
implementation is not ideal, and that we can and should improve it, just no=
t on how and in what version=2E

Oddly, the crux of this debate seems to be less that all options have majo=
r impact, and more that none of them do=2E If the implementation had caused=
 a serious security or performance regression, I don't think we'd have any =
hesitation in backing it out if a better implementation wasn't ready; or co=
ntrarily, if it had been a headline new feature, we wouldn't even be consid=
ering that option=2E

As it is, an improved implementation *is* proposed: https://github=2Ecom/p=
hp/php-src/pull/15041 Comparing this purely against 8=2E2, it seems a reaso=
nable compromise: consumers of the token stream still need to make a change=
, but it's the slightly more intuitive one of "treat yield and from as sepa=
rate tokens, which might have other tokens between"=2E Personally, I think =
it's reasonable to consider that a bug fix to the original change, and push=
 it into 8=2E3=2E

Projects using the token stream could detect the versions with the imperfe=
ct implementation, and emit an error explaining the "bug" if a T_YIELD_FROM=
 token doesn't match /yield\s+from/ Projects parsing the source code on the=
ir own will have to handle this however they handle contexts where comments=
 have always been allowed=2E

The Ubuntu LTS situation is unfortunate, but maybe Ond=C5=99ej Sur=C3=BD w=
ill have an opinion on what to do with the patch there=2E

Regards,
Rowan Tommins
[IMSoP]
------CYY9BGP63YI61GBYEIIZ8W6PLCDAQM
Content-Type: text/html;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable

<!DOCTYPE html><html><body><div dir=3D"auto">On 21 July 2024 12:25:27 BST, =
"Tim D=C3=BCsterhus" &lt;tim@bastelstu=2Ebe&gt; wrote:<br>&gt;The change ha=
s also been explicitly acked by Gina and Christoph before Ilija committed i=
t and Bob also participated in the PR without raising concerns, so it's als=
o not an unapproved change=2E The fact that none of them anticipated this s=
ide effect, doesn't make it unapproved=2E<br><br>I note that Gina commented=
: <br><br>&gt; This is kinda cursed=2E=2E=2E but sure why not=2E<br><br>And=
 Christoph wrote:<br><br>&gt; Let's say that I'm generally not happy with c=
omplicating the parser with work-arounds, but this is okay for me=2E<br><br=
>So, hardly a confident consensus that this was the right approach=2E Regar=
dless, we are where we are, and there's general agreement that the current =
implementation is not ideal, and that we can and should improve it, just no=
t on how and in what version=2E<br><br>Oddly, the crux of this debate seems=
 to be less that all options have major impact, and more that none of them =
do=2E If the implementation had caused a serious security or performance re=
gression, I don't think we'd have any hesitation in backing it out if a bet=
ter implementation wasn't ready; or contrarily, if it had been a headline n=
ew feature, we wouldn't even be considering that option=2E<br><br>As it is,=
 an improved implementation *is* proposed: <a href=3D"https://github=2Ecom/=
php/php-src/pull/15041">https://github=2Ecom/php/php-src/pull/15041</a>=C2=
=A0Comparing this purely against 8=2E2, it seems a reasonable compromise: c=
onsumers of the token stream still need to make a change, but it's the slig=
htly more intuitive one of "treat yield and from as separate tokens, which =
might have other tokens between"=2E Personally, I think it's reasonable to =
consider that a bug fix to the original change, and push it into 8=2E3=2E<b=
r><br>Projects using the token stream could detect the versions with the im=
perfect implementation, and emit an error explaining the "bug" if a T_YIELD=
_FROM token doesn't match /yield\s+from/ Projects parsing the source code o=
n their own will have to handle this however they handle contexts where com=
ments have always been allowed=2E<br><br>The Ubuntu LTS situation is unfort=
unate, but maybe Ond=C5=99ej Sur=C3=BD will have an opinion on what to do w=
ith the patch there=2E<br><br>Regards,</div><div dir=3D"auto">Rowan Tommins=
<br>[IMSoP]</div></body></html>
------CYY9BGP63YI61GBYEIIZ8W6PLCDAQM--