Hi Juliette On Sat, Jul 20, 2024 at 6:40 PM Juliette Reinders Folmer <php-internals_nos...@adviesenzo.nl> wrote: > > On 20-7-2024 18:04, Tim Düsterhus 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 into > T_YIELD T_WHITESPACE T_FROM_FOR_YIELD_FROM". > > Tim, you're making my point for me. This is *exactly* why the current change > should be reverted. > > I'm not against changing the tokenization of "yield from" and the GH ticket > 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 documented change. > > And not, like it is now, an undocumented, random change creating an > inconsistency 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
