-
Notifications
You must be signed in to change notification settings - Fork 236
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
Support for use statement resolution in annotation property types #131
Conversation
a63b0ee
to
13d5714
Compare
13d5714
to
2c90267
Compare
// partially namespaced annotation instance | ||
array( | ||
'partiallyNamedAnnotation', | ||
'Common\Annotations\Fixtures\AnnotationTargetAll', |
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 assumption is strange, it should be the FQCN, not the one used in declaration. It works fine with arrays - see arrayOf* tests below, but for non-array values, it doesn't seem to be correct...
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.
Fantastic work on the tests, but there is the problem of by-ref manipulation, plus the possible performance impact (possibly huge)
* | ||
* @return void | ||
*/ | ||
private function resolveUnqualifiedAttributeType(&$metadata, Attribute $attribute, ReflectionClass $class) |
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.
We should really avoid by-ref modification wherever possible
* | ||
* @return void | ||
*/ | ||
private function resolveUnqualifiedType(&$type, ReflectionClass $class) |
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.
Same: by-ref is a no-go here
This is a rather simple approach to implement #86 / doctrine/common#562, originally reported by @enumag ages ago.☺️
I don't know the internals of doctrine/annotations well so any suggestions are welcome.