-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Drop doctrine/lexer 2 #10332
Drop doctrine/lexer 2 #10332
Conversation
Do we have to drop Lexer 2 for this change or could we do the same with |
I thought like you and I've backported a lot of it to #10329 |
* {@inheritdoc} | ||
*/ | ||
protected function getType(&$value): TokenType | ||
protected function getType(string &$value): 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.
This is the change that we can't do without altering the version constraint
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.
Oh, because we didn't add parameter types in v2. That's unfortunate.
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.
If we did that then we would have to bump to 7.2 in order to drop lexer 1 on ORM 2, so all in all, I think things are better like this. I won't pretend I considered that before though 😅
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.
Can we remove the string
type for now, so we can keep ^2 || ^3
as constraint? I'm afraid that strictly requiring v3 might block the adoption of ORM 3 in the beginning. We can still drop v2 at a later time.
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'm AFK for 2 weeks, feel free to open another PR with just the ctor type hint, the rest can be type hinting, and we can decide what to do with this PR at the last minute
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'm in favour of ^3
. Version 2, as far as I understood, had the job to be a transition for such changes and is supposed to be a short lived release.
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.
Yes, but Lexer is not only used in the ORM. And I don't want to block the adoption of ORM 3 because of a single parameter type in a single method.
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.
Let's monitor the install stats of v3 once 2.15 is released, and of v2 starting now, it should give some insight about whether this is going to be a big blocker or not.
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.
2.15 is not even released yet, and the install stats of v3 are already rising, quite close to the install stats of v2: https://packagist.org/packages/doctrine/lexer/stats
I think this is fine to merge, although we might want to do it right before the release of ORM v3, so that we get more data points.
This allows us to have a totally typed lexer
This allows us to have a totally typed lexer