Conversation
|
That looks like the better solution, thank you! |
3af8fdc to
42e9f66
Compare
|
@iluuu1994 Do you think it's reasonable to merge this soon? After all, it's strictly superior to the current implementation - even if we decide to revert anything, we'd just revert this as well. |
|
Given there's an ongoing discussion on the list, let's not jump the gun and cause more drama. But we should make sure to merge it before feature freeze. |
|
Sure, soon, not now :-) So I agree. |
| REGISTER_LONG_CONSTANT("T_PRINT", T_PRINT, CONST_PERSISTENT); | ||
| REGISTER_LONG_CONSTANT("T_YIELD", T_YIELD, CONST_PERSISTENT); | ||
| REGISTER_LONG_CONSTANT("T_YIELD_FROM", T_YIELD_FROM, CONST_PERSISTENT); | ||
| REGISTER_LONG_CONSTANT("T_FROM", T_FROM, CONST_PERSISTENT); |
There was a problem hiding this comment.
Should T_YIELD_FROM be deprecated first (as unused as of this PR) and removed in 9.0 ?
There was a problem hiding this comment.
That doesn't really apply for tokenizer constants. They always match what exists for the specific PHP version.
(It's also a generated file.)
There was a problem hiding this comment.
Fair enough, just makes the BC break larger again.
|
@bwoebi I appreciate you opening this PR to improve the situation. Was there any bike-shedding done on what the tokenization should be ? I've seen several different ideas floating around, so is there a pertinent reason why this implementation is favoured over the other possible ones ? yield from gen();
yield /*comment*/ from gen();
Some alternative options:
Other questions:
|
|
I don't care about Also, tokenization is very explicitly going to change from minor to minor. I very clearly do not care about BC with respect to tokenization between minors. It's an internal detail of the parser, which we expose and guarantee stability for within a minor. From that perspective I am going towards the approach which does not carry any technical debt (like a T_YIELD_FROM token which occurs only in specific scenarios). I'm targeting PHP 8.4 due to the BC break. I personally don't mind changing it in PHP 8.3 too. However, you'll have to persuade internals about this. |
Sorry, but I find that statement a little disingenuous. Yes, tokens change in every minor due to new features - which always go via RFCs. The Tokenizer is part of the public API and is used by all sorts of tooling (yes, PHPCS, but also PHPUnit, Composer, PHP Parallel Lint, phpDocumentor etc etc), so BC-breaks should be treated the same as anywhere else and be clearly annotated and documented. How can tooling be expected to keep up and deal with it, if not ? |
|
Yes, the change should have been documented. But doing the change in the first place was perfectly fine. |
Fixes GH-14926.
Changing it in PHP 8.3 would be a BC break, but for PHP 8.4 it should be a proper fix.