[WIP] Implement reader using reflection

[mpajunen]

Intro

I have a use case where I'd like to use Swagger annotations in traits and I noticed that the parsing mechanism didn't support this. I thought using reflection to read the annotations would be easier anyway and this is what I came up with.

This PR contains an alternative, work in progress implementation of using reflection and Doctrine AnnotationReader to read the annotations instead of a token parser. It's not meant to be merged as is, but I wanted to get some feedback to decide how to proceed from here.

How it works

To the user reading the annotations works pretty much the same as currently. With default options, it'd look something like this:

use Swagger\Builder;
$builder = Builder::createDefault();
$swagger = $builder->parse('/my/project/dir');

The new Builder and Reader classes can be customized with dependency injection as needed.

Internally the build process works as follows:

1. Find files using Finder.
2. Find the PHP class in the file using TokenParser.
3. Use reflection and AnnotationReader to read class, method and property annotations from these files.
4. Process these annotations: Read property and class names from the related reflection objects etc.
5. Merge annotations from a single class into one AbstractAnnotation object.
6. Merge annotations from different classes into a single Swagger object.
7. Process the Swagger object.
8. Validate the Swagger object.

Steps 2 - 5 are mostly new, the rest are basically the same as using the current version.

Notes

• The FinderFactory class was extracted from Swagger and is used by both build processes.
• The reflection based reader does not use the Parser class.
• Instead of using the Context class, the annotations are wrapped in AnnotationContainer objects that also contain the matching reflection object. The class reflection could also be accessed when processing method and property annotations if needed.
• New processors are used to process the AnnotationContainer objects since token parsing is not needed in this case.

The new reader has basic functionality, and produces correct results for the included examples (with some additions, included). There are currently incompatibilities between the reader implementations, but at least some of them can be solved with some additional development:

• The reflecting reader does not consider stand-alone DocBlocks, only those attached to classes, methods or properties. While by definition annotations should annotate something, this is not a requirement currently in Swagger-PHP.
• Similarly, all annotation classes must available when reading annotations, or explicit blacklisting must be used.
• Since the Context class is effectively not used, the output from validation is not very useful.

How to proceed

There's probably more to consider, but overall I think that there are three options:

1. If this is too drastic a change / addition, I can split the reader into a new library. This library would then use Swagger-PHP as a dependency for the annotation classes.
2. Since some of the incompatibilities are quite fundamental, reflecting reader could be added to Swagger-PHP as an optional feature. With further refactoring, it would be possible to increase code reuse between the readers.
3. If the incompatibilities are not an obstacle, using reflection and AnnotationReader could replace the current reader implementation entirely with some additional development. The immediate benefit would be trait support, but not having the own Parser implementation would also mean a lot less code to maintain in the long run.

In any case, I'm just looking for some overall thoughts at this point.

[bfanger]

Reflection pro's	Reflection con's
Robust trait and inhertitance support	Needs to executes arbitrary php code.
Easier to maintain when new language features are added to PHP.	Ignores phpdoc blocks not connected to a class, method or property
	Requires a classloader to autoload related classes and traits.

Swagger-PHP used to work based on reflection. It was actually my 3rd pull request that switched it to static analyses :)

But the PHP landscape has changed a lot since then, so maybe its time to revisit the reflection strategy.

Option 1: new lib

I'd like you to work on swagger-php directly, but a separate library is certainly an option.

Option 2: both

• Swagger::crawl should indeed be its own thing (I think i'll merge Builder and FinderFactory into a Crawler class).
• Rename Parser to StaticAnalyser (to make room for the Runtime/ReflectionAnalyser)
• Move the Parser::parseContext() logic to Analyser::fromComment()
• A Context can be generated from a ReflectionClass or ReflectionProperty. So the ReflectionAnalyser should be able to generate the same output type as the StaticAnalyser. Although the ReflectionAnalyser needs less postprocessing.

Option 3: reflection only

I'd could be Loss aversion, but i'm not ready to ditch the parser just yet.
I've found the Doctrine's AnnotationReader lacking in the past.
I'd rather use option 2, then make it the default choice when it's a clear improvement. eventually mark static as deprecated and then remove it.

Option 4: processor

Building a ReflectionProcessor that uses ReflectionClass to determine inheritance and traits.

Option 5: no reflection

For traits and inheritance using purly static analysis swagger-php needs to create a worldview object that contains all annotations, all classes and traits and their relationships. I think the processors should then operate on this worldview object instead of an Swagger object.

Closing thoughts.

One of the swagger-php 2.x goals is to simplify the architecture/api/codebase.
Option 3 requires less code, but at the expense of features and stability.
Option 2 adds complexity, but i don't mind changing the api to make option 1 easier.
Option 4 is the easiest way to get trait support while using the static analyser, but option 5 is my preferred option.

[mpajunen]

Thanks for the comments.

I'm not sure what "Needs complex logic to figure out traits and inheritance" means in the reflection cons list -- since there's actually no extra logic.

The earlier issues the "Needs to executes arbitrary php code." comment refers to look nasty indeed. Though it seems they are related to buggy autoloader implementations rather than reflection directly. Composer has also been developed significantly since then. I've tested the annotation reader with a couple of large projects and noticed no problems (still anecdotal of course).

In any case I'll refactor this PR to follow the Analyser structure and I guess we'll see what would need to be done after that.

[mpajunen]

Basic refactoring is now done:

• Reader was renamed, now ReflectingAnalyser. The functionality now matches StaticAnalyser.
• A common interface AnalyserInterface was added for the analysers.
• Builder was renamed, now Scanner. It has basically the same functionality as the scan function, but the environment can be customized. It can be used with either analyser implementation.
• Some other classes were renamed as well.

[mpajunen]

There was a problem here: If the Swagger annotation being merged also contained unmerged annotations, those annotations were lost. This is the quick single line fix.

The whole merging process seems quite brittle and may be going away anyway with the world view objects and what not, so I'm not sure if any additional effort should be spent here. I can at least add a test for this case though.

[GuilhemN]

Do you need help to finalize this or isn't this planned at all? I'd like to extend Swagger-php with custom annotations/processors to integrate it in Symfony but I currently can't without using my annotation's fqcn everytime as custom imports aren't detected.

[bfanger]

@GuilhemN A reflection based analyser is not going to help with custom annotations or processors.
I could move ['swg' => 'Swagger\Annotations'] into a public static variable, Swagger\Analyser::$defaultImports for example, that way you'd be able to register a custom namespace alias.

In the mean time you don't have to use the fqcn for every annotation, swagger tricks doctrine as if the file contains: use Swagger\Annotations as SWG;

You can for example add use MySymfonyNamespace\Annotations as SNY; in the beginning of the file and use @SNY\MyAnnotation() in the docblocks.

[GuilhemN]

@bfanger sure that would help, even if the best would be to support the user imports (as everyone has his own habits).

In the mean time you don't have to use the fqcn for every annotation, swagger tricks doctrine as if the file contains: use Swagger\Annotations as SWG;

Yes, I meant only when using custom annotations but that's already anoying.

[bfanger]

Looking forward to this "user imports" as a pull request 😉

[GuilhemN]

@bfanger in fact it is already supported, I just had to add my namespace to Analyser::$whitelist 😶
Thanks for this great library !

[mpajunen]

This was kind of left in a limbo. For a while I tried keeping it compatible with other changes, but that was somewhat tricky. Then we pretty much stopped using Swagger-php, for the most part it was replaced with a library similar to the Symfony PropertyInfo component) + a custom writer, so I lost interest in this PR as well. I also wasn't quite sure how to proceed and what (if any) implementation might be an acceptable PR, and didn't bother to ask for clarification.

In case there's interest for reviving this -- say, to help with #360 -- go for it. I might be able to contribute something to finish this as well, but can't promise anything.