Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:124532
Precedence: bulk
MIME-Version: 1.0
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>
In-Reply-To: <669BE870.2050908@adviesenzo.nl>
Date: Sun, 21 Jul 2024 13:28:34 +0200
Message-ID: <CAPyj-LB538drkCdq2jbrAWuU5uTq_izW97ppMvN=X=xDPfCZUQ@mail.gmail.com>
Subject: Re: [PHP-DEV] Request for opinions: bug vs feature - change
 intokenization of yield from
To: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
From: tovilo.ilija@gmail.com (Ilija Tovilo)

Hi Juliette

On Sat, Jul 20, 2024 at 6:40=E2=80=AFPM Juliette Reinders Folmer
<php-internals_nospam@adviesenzo.nl> wrote:
>
> On 20-7-2024 18:04, Tim D=C3=BCsterhus wrote:
>
> As I've said: I agree that the current situation is unfortunate. But the =
correct solution is not "disallow comments", but "split the T_YIELD_FROM in=
to T_YIELD T_WHITESPACE T_FROM_FOR_YIELD_FROM".
>
> Tim, you're making my point for me. This is *exactly* why the current cha=
nge should be reverted.
>
> I'm not against changing the tokenization of "yield from" and the GH tick=
et thread also contains an alternative proposal for this from Bob [1], but =
like Matthew also said [2]: if that's something we want to do, let's have a=
 proper discussion about it and let it go through an RFC and be a documente=
d change.
>
> And not, like it is now, an undocumented, random change creating an incon=
sistency in the Tokenizer.

Thank you for raising this issue. Let me share my viewpoint as the
person who has made the change.

First of all, while allowing comments between yield and from was
obviously intentional, the side-effect of embedding the comment in the
yield_from token was something I did not sufficiently consider. That's
my bad.

I agree that it's not very likely that many people started placing
comments between yield and from, but certainly not impossible. The
main problem with reverting this change is that such code will
completely stop working. Patch updates are routinely installed on
servers without additional testing, so it's very much possible that we
would brick peoples production environments.

To compare the impact on PHP_CodeSniffer: From what I understand
(please correct me if I'm wrong), there are really only two possible
scenarios when encountering comments between yield and from:

* PHP_CodeSniffer doesn't see the comment and acts as if it weren't
there, and potentially removes it when fixing errors automatically.
* PHP_CodeSniffer thinks this is a syntax error and thus reports it.

While that's not optimal, it is limited to a subset of the already
small set of people using this new feature, namely the ones also using
PHP_CodeSniffer. It is also limited to development environments, where
PHP_CodeSniffer is executed.

What I'd suggest instead is fixing this for master in a way that makes
it simple for PHP_CodeSniffer to add support, or to just revert it
there. If it is indeed true that nobody uses this feature after 8
months, then waiting another 4 for a proper fix doesn't seem like an
issue.

In the likely case of not finding consensus on this issue: Setting up
a vote doesn't seem straight-forward to me either. Would we consider
8.3 the baseline, with reverting the change requiring the 2/3
majority, or do we consider 8.2 the baseline?

Ilija