Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:124541
Subject: Re: [PHP-DEV] Request for opinions: bug vs feature - change
 intokenization of yield from
To: internals@lists.php.net
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>
 <2C00EB00-0BA4-40F8-92A5-50B49A639AD6@rwec.co.uk>
Message-ID: <669D5467.4090103@adviesenzo.nl>
Date: Sun, 21 Jul 2024 20:33:11 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:38.0) Gecko/20100101
 Thunderbird/38.7.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <2C00EB00-0BA4-40F8-92A5-50B49A639AD6@rwec.co.uk>
Content-Type: multipart/alternative;
 boundary="------------000400010406010907080308"
From: php-internals_nospam@adviesenzo.nl (Juliette Reinders Folmer)

This is a multi-part message in MIME format.
--------------000400010406010907080308
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit

On 21-7-2024 17:11, Rowan Tommins [IMSoP] wrote:
> Oddly, the crux of this debate seems to be less that all options have 
> major impact, and more that none of them do. 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 contrarily, if it had been a headline new feature, we 
> wouldn't even be considering that option.

The crux - to me - is that it is an undocumented breaking change, which 
by definition is a bug.

As I've said before, I'm not against changing the tokenization, what I'm 
speaking up about is that it was done in an inconsistent, semi-random 
and undocumented way.

> As it is, an improved implementation *is* proposed: 
> https://github.com/php/php-src/pull/15041 Comparing this purely 
> against 8.2, it seems a reasonable 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 separate tokens, which might 
> have other tokens between". Personally, I think it's reasonable to 
> consider that a bug fix to the original change, and push it into 8.3.
>
> Projects using the token stream could detect the versions with the 
> imperfect 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 their own will have to handle this however they handle 
> contexts where comments have always been allowed.

I agree the improved tokenization proposed in the PR makes better sense.

Having said that, it will make the breaking change much much larger as 
it now would no longer just be a change which can be handled in the 
PHPCS token stream, but one which will impact individual sniffs, what 
with the removal and introduction of new tokens.

If that PR gets merged, it will mean that PHPCS will need to undo the 
PHP 8.3 and PHP 8.4 tokenization for the time being (in the 3.x major) 
and will only "polyfill" the new tokenization as of PHPCS 4.0. In 
practice, we would be moving the breaking change to PHPCS 4.0 version to 
not break sniffs.

Smile,
Juliette


--------------000400010406010907080308
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: 8bit

<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <div class="moz-cite-prefix">On 21-7-2024 17:11, Rowan Tommins
      [IMSoP] wrote:<br>
    </div>
    <blockquote
      cite="mid:2C00EB00-0BA4-40F8-92A5-50B49A639AD6@rwec.co.uk"
      type="cite">
      <div dir="auto">Oddly, the crux of this debate seems to be less
        that all options have major impact, and more that none of them
        do. 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
        contrarily, if it had been a headline new feature, we wouldn't
        even be considering that option.<br>
      </div>
    </blockquote>
    <br>
    The crux - to me - is that it is an undocumented breaking change,
    which by definition is a bug.<br>
    <br>
    As I've said before, I'm not against changing the tokenization, what
    I'm speaking up about is that it was done in an inconsistent,
    semi-random and undocumented way.<br>
    <br>
    <blockquote
      cite="mid:2C00EB00-0BA4-40F8-92A5-50B49A639AD6@rwec.co.uk"
      type="cite">
      <div dir="auto">As it is, an improved implementation *is*
        proposed: <a moz-do-not-send="true"
          href="https://github.com/php/php-src/pull/15041">https://github.com/php/php-src/pull/15041</a> Comparing
        this purely against 8.2, it seems a reasonable 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
        separate tokens, which might have other tokens between".
        Personally, I think it's reasonable to consider that a bug fix
        to the original change, and push it into 8.3.<br>
        <br>
        Projects using the token stream could detect the versions with
        the imperfect 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 their own will have to
        handle this however they handle contexts where comments have
        always been allowed.<br>
      </div>
    </blockquote>
    <br>
    I agree the improved tokenization proposed in the PR makes better
    sense.<br>
    <br>
    Having said that, it will make the breaking change much much larger
    as it now would no longer just be a change which can be handled in
    the PHPCS token stream, but one which will impact individual sniffs,
    what with the removal and introduction of new tokens.<br>
    <br>
    If that PR gets merged, it will mean that PHPCS will need to undo
    the PHP 8.3 and PHP 8.4 tokenization for the time being (in the 3.x
    major) and will only "polyfill" the new tokenization as of PHPCS
    4.0. In practice, we would be moving the breaking change to PHPCS 
    4.0 version to not break sniffs.<br>
    <br>
    Smile,<br>
    Juliette<br>
    <br>
  </body>
</html>

--------------000400010406010907080308--