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
DX: Allow doctrine/annotations 2 #6721
Conversation
2e8227b
to
b2eec69
Compare
Pull Request Test Coverage Report for Build 4039091970
💛 - Coveralls |
composer.json
Outdated
@@ -19,7 +19,8 @@ | |||
"ext-tokenizer": "*", | |||
"composer/semver": "^3.2", | |||
"composer/xdebug-handler": "^3.0.3", | |||
"doctrine/annotations": "^1.13", | |||
"doctrine/annotations": "^1.13 || ^2", |
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.
would the current CI setup have a job that run against v1, and another job that runs against v2 ?
also, can we support only v2?
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.
would the current CI setup have a job that run against v1, and another job that runs against v2 ?
Yes, your low-deps job runs with Annotations 1.13, all the other jobs run with 2.0.
also, can we support only v2?
You certainly can. However, many projects have Doctrine Annotations installed as a transitive dependency. If they install CS Fixer via require-dev
, dropping support for v1 right away might cut them off from new CS Fixer releases until all of their dependencies are compatible with v2.
Supporting both major branches means hardly any extra effort on your side and you'd hardly gain anything from dropping v1. My suggestion would be to revisit this dependency in a couple of months and drop v1 once the adoption of v2 is higher.
b2eec69
to
2ea503b
Compare
@@ -19,7 +19,8 @@ | |||
"ext-tokenizer": "*", | |||
"composer/semver": "^3.2", | |||
"composer/xdebug-handler": "^3.0.3", | |||
"doctrine/annotations": "^1.13", | |||
"doctrine/annotations": "^1.13 || ^2", | |||
"doctrine/lexer": "^1 || ^2", |
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.
why we need to explicitly mention this peer dependency ?
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.
PHP CS Fixer does not rely on Doctrine\Common\Lexer
directly. why we need to mention it [or why we need to prevent v3]?
shouldn't it be covered by whatever using this library?
If behaviour of Doctrine/Annotation v2 is different, depends on version of it's own dependency [lexer], sounds like issue on Annotation package, imho.
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.
PHP CS Fixer does not rely on
Doctrine\Common\Lexer
directly.
It kinda does by using Doctrine\Common\Annotations\DocLexer
which extends a class from the Lexer library.
why we need to mention it
Because Annotations 2 would allow the installation of Lexer 3 which requires some code changes on CS Fixer's side. I can make those adjustments if you want.
The main issue is that Lexer 2 replaces the associative array structure of tokens with a dedicated Token
class. We access that array structure here for instance:
if (DocLexer::T_OPEN_PARENTHESIS === $token['type']) { |
In Lexer 1, this property needs to be accessed via $token['type']
while Lexer 3 requires us to write this as $token->type
instead. Lexer 2 allows both ways by implementing ArrayAccess
as a compat layer. There are no new features in Lexer 3 and Doctrine has not even transitioned their own projects to Lexer 3 yet, so there's no urgency in supporting it just yet.
Our options are:
- Keep this PR as is and work on Lexer 3 compat with a follow-up. Annotations 1 blocks the installation of Lexer 3 anyway, so this new line in composer.json wouldn't generate a conflict that isn't already there.
- Remove the new constraint and support Lexer 1, 2 and 3: This will make the codebase a little more complicated, but I think I can bury that complexity in a private method.
- Require Lexer
^2 || ^3
. No multi-version hacks and the codebase is future-proof, but might cause downstream conflicts at the moment.
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 am totally shocked that I can install annotations v2 and get totally different interface as return [sometimes assoc array, sometimes DTO] ;/
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.
https://packagist.org/packages/doctrine/annotations/stats#major/all
v2 not used at all, yet
https://packagist.org/packages/doctrine/lexer/stats#major/all
v1, a bit v2, no v3 yet
IMHO:
- annotations: v1 || v2, lexer: v1 || v2
- let's prepare 2 PRs. one that is allowing lexer v1 || v2 (this PR), other that is allowing only v2.
- let's release first PR as vX.Y.Z, and 2nd PR as vX.Y.(Z+1). then, we see how many of our users will stuck with Z and not able to upgrade to Z+1.
- and in case of need, PR with v2 only will be easy to revert, if there will be huge gap between Z and Z+1
- and if all good, we can extend it to v2 || v3 and use future-ready interface only
- let's release first PR as vX.Y.Z, and 2nd PR as vX.Y.(Z+1). then, we see how many of our users will stuck with Z and not able to upgrade to Z+1.
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.
Sounds like a plan. I'll prepare the second PR.
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.
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.
Thanks !
It will take me a while to merge [ I want to merge first the bugfixes first, and then release those deps compatibility on top ]
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 see. I'm currently submitting this kind of PR to several open-source libs to push the adoption of the new Lexer and Annotations releases. Some of them are currently impeded by CS-Fixer.
Merging (and releasing) this PR at least would enable me to continue my work.
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 am totally shocked that I can install annotations v2 and get totally different interface as return [sometimes assoc array, sometimes DTO] ;/
That's not the case actually, with annotations v2 you always get a Token
object. But you're right in that it's a different interface, because that object implements ArrayAccess
with lexer v2, and no longer does with lexer v3.
This PR allows to use PHP CS Fixer with
doctrine/annotations
2. However, the PR disallows the installation ofdoctrine/lexer
3 for the moment because that would require major code changes. I can work on that in a follow-up if you like.