-
-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace object-like array with class #79
Conversation
lib/Doctrine/Common/Lexer/Token.php
Outdated
* @template T of string|int | ||
* @implements ArrayAccess<string,mixed> | ||
*/ | ||
final class Token implements ArrayAccess, Countable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I will remove Countable
, as ArrayAccess
should be enough to easily implement a forward-compatibility layer.
e99786b
to
65f3cdc
Compare
* @template T of string|int | ||
* @implements ArrayAccess<string,mixed> | ||
*/ | ||
final class Token implements ArrayAccess |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although no BC-layer will be provided, implementing ArrayAccess
should allow to easily make dependent packages compatible with lexer 1 and 2 at the same time.
We should try to ship this change in 1.x. It would be unfortunate if we needed to ship 2.x with a BC layer. |
I think we are going to need a 3.x very soon, because inheritance is involved: the ORM lexer extends the AbstractLexer, which means that if we add return type hints on 3.x, we will make it impossible for doctrine/orm to be compatible with both v1 and v2 at the same time, just like here: doctrine/collections#331 For that reason, I think it's not that big of a deal to have that BC layer on 2.x, since that series is supposed to be short-lived. If you can think of a way to ship this change in 1.x, I'm still interested though. |
I'm working on a solution to ship this in 1.x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that one. Having 2.x as an interim version sounds reasonable.
The new class is templatable, which should enable us to specify that the ORM Lexer is an Lexer<self::T_*>, and in the future, define an enum called TokenType in the ORM, and switch to Lexer<TokenType>.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to hear what other reviewers have to add.
ping @derrabus are you fine with this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a better idea right now. 🙂
} | ||
|
||
/** @param T ...$types */ | ||
public function isA(...$types): bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn’t this be inA() ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi mate! No it's isA
, just like https://www.php.net/manual/en/function.is-a.php (but with several types) 🙂
By requiring `doctrine/lexer` v2 we can solve the following deprecation: ``` Accessing Doctrine\Common\Lexer\Token properties via ArrayAccess is deprecated, use the value, type or position property instead (Token.php:104 called by Parser.php:46, doctrine/lexer#79, package doctrine/lexer) ```
By requiring `doctrine/lexer` v2 we can solve the following deprecation: ``` Accessing Doctrine\Common\Lexer\Token properties via ArrayAccess is deprecated, use the value, type or position property instead (Token.php:104 called by Parser.php:46, doctrine/lexer#79, package doctrine/lexer) ```
By requiring `doctrine/lexer` v2 we can solve the following deprecation: ``` Accessing Doctrine\Common\Lexer\Token properties via ArrayAccess is deprecated, use the value, type or position property instead (Token.php:104 called by Parser.php:46, doctrine/lexer#79, package doctrine/lexer) ```
By requiring `doctrine/lexer` v2 we can solve the following deprecation: ``` Accessing Doctrine\Common\Lexer\Token properties via ArrayAccess is deprecated, use the value, type or position property instead (Token.php:104 called by Parser.php:46, doctrine/lexer#79, package doctrine/lexer) ```
The new class is templatable, which should enable us to specify that the
ORM Lexer is an
Lexer<self::T_*>
, and in the future, define an enumcalled TokenType in the ORM, and switch to
Lexer<TokenType>
.