Skip to content
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

use namespace from traits #22

Merged
merged 5 commits into from
Feb 3, 2014
Merged

Conversation

jaypea
Copy link

@jaypea jaypea commented Dec 20, 2013

This adds support for Annotated Methods in Traits and is a follow up and replacement for PR #20

Annotations inside of Traits can use different Namespace than the including Class, see ClassUsesTrait.php in this PR for example.
As Methods of Traits can be overwritten, the Namespace of the actual class needs to be used.

Known Issue:
In the rare case of having the Trait and the Class defined in the same File and a method from Trait is overwritten in the Class and the namespace of an annotation differs than the namespace from the trait is used instead of the correct one from the class.

@@ -10,4 +10,29 @@ protected function getReader()
{
return new AnnotationReader();
}

public function testAnnotationFromTrait()
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shoudln't you skip the test on 5.3 ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx, changed it.

@stof
Copy link
Member

stof commented Dec 20, 2013

This should also be supported for properties defined in traits

@jaypea
Copy link
Author

jaypea commented Dec 20, 2013

@stof implemented propertyImports

@stof
Copy link
Member

stof commented Dec 22, 2013

Please add a test with annotations on a property in a trait

@jaypea
Copy link
Author

jaypea commented Dec 22, 2013

@stof test added. I'm glad you asked, found an error in the code because of the test ;)

@stof
Copy link
Member

stof commented Dec 23, 2013

Looks good to me, but I don't have merge rights on this Doctrine repo

@jaypea
Copy link
Author

jaypea commented Feb 3, 2014

@beberlei, @Ocramius is this ready to merge?

beberlei added a commit that referenced this pull request Feb 3, 2014
@beberlei beberlei merged commit 2ee2bc8 into doctrine:master Feb 3, 2014
@jaypea jaypea deleted the trait_support branch February 10, 2014 09:11
@Ocramius Ocramius added this to the v1.2.0 milestone Jul 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants