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

Support ::class notation #360

Closed
GuilhemN opened this issue Jan 16, 2017 · 12 comments
Closed

Support ::class notation #360

GuilhemN opened this issue Jan 16, 2017 · 12 comments

Comments

@GuilhemN
Copy link
Contributor

It is currently not possible to use the ::class syntax as the Parser limits the allowed class of the DocBlockParser.
Wdyt about trying to parse the doc blocks with all namespaces supported and fallback to a limited subset of namespaces when the parsing fails?
Imo we should not support having undeclared annotations in files (eventually silently fail? or trigger a notice?), and it should be deprecated.

I can submit a PR if you agree with this.

@bfanger
Copy link
Collaborator

bfanger commented Jan 16, 2017

What is not possible with the ::class syntax? Could you show some example code?

@GuilhemN
Copy link
Contributor Author

GuilhemN commented Jan 16, 2017

As I told you previously, i'm using a custom annotation on top of the library which requires a FQCN:

use App\Foo;

/**
  * @Model(Foo::bar)
  */

But this example doesn't work

@bfanger
Copy link
Collaborator

bfanger commented Jan 16, 2017

This looks more like a doctrine issue than a swagger-php issue.

Swagger-php is designed to parse (but not execute) any php codebase and extract swagger documentation. Therefore we can't generate warnings on undeclared annotations, if doctrine has a strict mode let me know, that could be useful as an optional cli parameter

@mpajunen
Copy link
Contributor

I think the issue here isn't the annotation class, but the class constant Foo::bar. Last I checked Swagger-php does not support using class constants inside annotations, even if all the correct namespace declarations are present. This was one major reason for my earlier, unfinished reflection based reader in #214 since Doctrine annotation reader supports this out of the box.

@GuilhemN
Copy link
Contributor Author

@mpajunen indeed you're right, we can't use constants at all. Having a reader based on reflection would be a good move imo but maybe too big at once.

The part of your PR I'd like to see is:

Similarly, all annotation classes must available when reading annotations, or explicit blacklisting must be used.

It sounds weird to me to consider annotations in invalid files. Imo we should either throw a warning or silently ignore them.

@bfanger
Copy link
Collaborator

bfanger commented Jan 18, 2017

Reflection must execute a script before it can reflect.

How would swagger-php know that "my-reset-database-script.php" and "deploy-to-production-script.php" are "invalid" files?

Normal php constants work when they are defined before scanning, thats why the cli has a --bootstap option. Maybe the thats the same for class constants, but i'll have to look into that.

@GuilhemN
Copy link
Contributor Author

Class constants work only when they are fully qualified which is obviously not convenient.

Reflection must execute a script before it can reflect.

How would swagger-php know that "my-reset-database-script.php" and "deploy-to-production-script.php" are "invalid" files?

The subject here is about invalid annotations, not especially about reflection.
For invalid annotations we only have to catch any exceptions trigerred by the parser.

bfanger added a commit that referenced this issue Jan 23, 2017
@bfanger
Copy link
Collaborator

bfanger commented Jan 23, 2017

I've researched the issue and you're right, the Swagger\StaticAnalyser only reads the use statements that are in the Analyser::$whitelist. The others are ignored, causing the [Semantical Error] Couldn't find constant Foo::bar warning.

I've added a new Analyser::$whitelist=false mode. This will cause the Swagger\StaticAnalyser to read all the use statements and fix the class constants issue.
An alternative solution is to add the namespace to Analyser::$defaultImports array.

Sadly reading all use statements can result in [Semantical Error] The annotation "@$annotation" in $filename on line $line does not exist, or could not be auto-loaded.
Take "could not be auto-loaded" with a bucket of salt, because it also applies to classes that can be autoloaded by composer or php's build-in autoloading.
You'll need to configure Doctrine\Common\Annotations\AnnotationRegistry to load those annotation classes.

It isn't possible to whitelist annotations in the DocParser, but it is possible to blacklist annotations.
Any idea's to solve class constants without introducing annotation errors?

@GuilhemN
Copy link
Contributor Author

I'm thinking of a way to use Analyser::$whitelist=false for my use case, it will be more limited but easier to use.
It should be enough for me.

More globally, after thinking again about it, I think having a reflection based reader could be a good move.

Reflection must execute a script before it can reflect.

How would swagger-php know that "my-reset-database-script.php" and "deploy-to-production-script.php" are "invalid" files?

In fact I just realized #214 do not use reflection unless a file name is found, and reflection only works if the class is in the autoloader (and there is no reason to put my-reset-database-script.php in the autoloader): so there is no risk on this side.
Only classes file are put in the autoloader so nothing risky must be executed.

Imo the only thing missing to #214 is a way to silently ignore invalid classes (missing parent, missing annotations, etc)

@GuilhemN
Copy link
Contributor Author

Well this issue is fixed for me (I have my workaround thanks to fca06ec).
Should I let it opened as a reminder to fix the issue for everyone or should I close it?

@bfanger
Copy link
Collaborator

bfanger commented Feb 22, 2017

Thanks for the reminder, i've just released 2.0.9.

@bfanger bfanger closed this as completed Feb 22, 2017
@GuilhemN
Copy link
Contributor Author

Thanks !

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

No branches or pull requests

3 participants