Annotations 2.0: Read annotations from functions

[lennerd]

I know, this is maybe a strange way to add this functionality, but as this changes should be on top of doctrine#75 I think it's best to make this PR agains this forked repository to be able to focus on the right changes.

Mind that this is also a very big WIP in the title, as I really need to see what are the Do's and Dont's in the doctrine family. I'm really happy to hear, what you think and open to every kind of feedback.

Fixes: doctrine#83 (comment)

• Read annotations also from functions
• Write additional tests
• Gather feedback

[lennerd]

Tried my best to get better coveralls ratings. Happy about feedback when testing the CachedReader for example.

[lennerd]

@guilhermeblanco @FabioBatSilva Any news on this?

[guilhermeblanco]

@lennerd I'll revisit this branch as soon as start on the ClassMetadata dump support on ORM.
We're like 3-4 weeks away from that

[lennerd]

Whats the status here?

[FabioBatSilva]

Currently CachedReader relies on both interfaces FunctionReader and ObjectReader.
That doesn't seems right.

Having {Function,Object}Reader makes sense to me
but i'm not convinced we need separate implementations for each one of them..

I would suggest something like :
AnnotationReader implements FunctionalReader, ObjectReader {}
CachedReader implements FunctionalReader, ObjectReader {}

[lennerd]

@FabioBatSilva Thanks for the feedback! I'm a little confused however.

class AnnotationReader implements FunctionalReader, ObjectReader {}
class CachedReader implements FunctionalReader, ObjectReader {}

Isn't that a little contrary to the following?

Currently CachedReader relies on both interfaces FunctionReader and ObjectReader.
That doesn't seems right.

And why would you not have the CachedReader implement both the FunctionReader and ObjectReader? What would you propose instead?

[FabioBatSilva]

That is correct CachedReader implements both interfaces..
But the delegate property also relies on both interfaces FunctionReader and ObjectReader.
For instance, if you inject a FunctionReader and try to read annotations from a object/class it will fail.

I think we need to either segregate the implementations for {Function,Object}Reader including the CachedReader or have just a single implementation.

Single implementation:

interface Reader extends FunctionalReader, ObjectReader {}

class AnnotationReader implements Reader {}

class CachedReader implements Reader
{
    /** @var \Doctrine\Annotations\Reader */
    private $delegate;
}

Split implementations:

class FunctionalAnnotationReader implements FunctionalReader {}

class ObjectAnnotationReader implements ObjectReader {}

class CachedFunctionalReader implements FunctionalReader {}

class CachedObjectReader implements FunctionalReader {}

[lennerd]

Ah I see what you mean.

I think I like the first approach to be much simpler and more compliant with the current implementation.

Will try this and come back to you.

[lennerd]

I just added the single implementation approach you were describing. Thanks again for the feedback. Quite happy with the simplifications in the CacheReader.

Again I would be happy about any kind of thoughts and feedback.

[lennerd]

@FabioBatSilva @guilhermeblanco Any more feedback?

[lennerd]

Nearly a year ago we started this. Any news or plans? Just looked through the code and I'm really impressed by the many improvements you added (especially the hoa parser) and how easy it was for me to add annotations for functions. 😕

[FabioBatSilva]

Hi @lennerd ..

Thanks for your work here and sorry for the looooong delay..
I can merge this if your are ok with it.

I'm not sure when 2.0 is going to happen.
But I know There is some work being done on doctrine#193 to get this merged step by step

[lennerd]

Hey @FlavioBatSilva,

I can merge this if your ok with it.

if you agree with all changes, that would be great! 😉

Great to see that the work for 2.0 is starting again! 👍

[guilhermeblanco]

@FabioBatSilva WHY U NO MERGE THEN?

PS: In your own fork, I'm sayin'...

[lennerd]

😄

[FabioBatSilva]

My Bad.. 😜

Thanks @lennerd !