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

Add missing imports of annotations. #7907

Closed
wants to merge 2 commits into from
Closed

Add missing imports of annotations. #7907

wants to merge 2 commits into from

Conversation

michaljusiega
Copy link

Hello.

It's my first PR for ORM project.
I found (I think) a some little issue with annotations. Using defualt new AnnotationReader(), we can get a error about missing import like:

Doctrine\Common\Annotations\AnnotationException : [Semantical Error] The annotation "@Enum" in property Doctrine\ORM\Mapping\Cache::$usage was never imported. Did you maybe forget to add a "use" statement for this annotation?

This PR fix this on 2.6

@greg0ire
Copy link
Member

I haven't made my mind about this PR yet, but there is no mention of that import in the docs: https://www.doctrine-project.org/projects/doctrine-annotations/en/latest/custom.html#enumerated-values

@michaljusiega
Copy link
Author

So we should leave it as it is still generating an exception?

@greg0ire
Copy link
Member

I haven't made my mind about this PR yet. Please provide a full stack trace, and tell us which version of the ORM and the annotations library you use. Maybe a recent update broke something? The files you propose to change are quite old, hence this hunch.

@michaljusiega
Copy link
Author

A complicated matter. Every ORM update since 2.6.0 I have to add these lines of code to make the whole project live for me. It's difficult for me to determine stack-trace and the cause of the error.

But it probably wouldn't hurt to add a few lines of code that theoretically do not change anything directly in the project.

But what's up, it's not my project. I reported such a need, because I was tired of modifications in this library so that everything was okay.

Yeah, the files are old ... because you are messing with this ORM 3.0 :D

@greg0ire
Copy link
Member

It's difficult for me to determine stack-trace and the cause of the error.

Why?

But what's up, it's not my project. I reported such a need, because I was tired of modifications in this library so that everything was okay.

And that's great, thank you ! I hope you can understand that we want to fully understand why there is a need for what me merge though. I even think that as a user, you would be quite afraid of using Doctrine if we didn't try to get to the bottom of things, wouldn't you?

@michaljusiega
Copy link
Author

No, I'm not afraid of using Doctrine. I have written tests for this in my own projects for which this error occurs - so if I do not notice the error after the update by Composer, the tests will tell the truth.

I added a test, but there is another error.

Instead:
Doctrine\Common\Annotations\AnnotationException : [Semantical Error] The annotation "@Enum" in property Doctrine\ORM\Mapping\Cache::$usage was never imported. Did you maybe forget to add a "use" statement for this annotation

got now:

Doctrine\Common\Annotations\AnnotationException : [Semantical Error] The annotation "@Doctrine\Common\Annotations\Annotation\Enum" in property Doctrine\ORM\Mapping\Cache::$usage does not exist, or could not be auto-loaded.

I'm confused :/

@greg0ire
Copy link
Member

greg0ire commented Nov 18, 2019

🤔 meanwhile, I had a look at this, and now my understanding is that we have been wrongly assuming 100% of people are using https://github.com/doctrine/annotations/blob/1.9/lib/Doctrine/Common/Annotations/SimpleAnnotationReader.php, when in fact they could be using https://github.com/doctrine/annotations/blob/1.9/lib/Doctrine/Common/Annotations/AnnotationReader.php

I have yet to understand why there are 2 classes and the difference(s) between the 2 (git doesn't help much this time, and the commit that introduced SimpleAnnotationReader was really hard to find: doctrine/common#49)

Your test looks good, ATM Travis is only complaining about coding style, no idea why you are getting this error.

@SenseException
Copy link
Member

The SimpleAnnotationReader was deprecated and will be removed in later versions because of its "magic" kind of working:

@beberlei
Copy link
Member

beberlei commented Dec 7, 2019

Why is the import needed for @Enum but not for the others like @Annotation and @Target? This feels like a fix for the symptom and not the problem.

@michaljusiega
Copy link
Author

So ... what are we doing with it? We close and forget? :)

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