Missing annotations with the latest composer version #120
Conversation
| @@ -728,6 +728,11 @@ private function Annotation() | |||
| } | |||
| } | |||
|
|
|||
| // strip the first \ off if used | |||
| if( '\\' == $name[0]) { | |||
| $name = substr($name,1); | |||
| @@ -108,4 +110,23 @@ public function testPropertyAnnotationIsIgnored() | |||
|
|
|||
| self::assertEmpty($reader->getPropertyAnnotations($ref->getProperty('property'))); | |||
| } | |||
|
|
|||
| /** | |||
| * @group gnat | |||
|
|
||
| $annotations = $reader->getClassAnnotations($ref); | ||
| } catch(AnnotationException $exception) { | ||
| $this->fail('Class with full path use statement unable to load annotation'); |
There was a problem hiding this comment.
If you don't expect an exception, let it bubble up
| $this->fail('Class with full path use statement unable to load annotation'); | ||
| } | ||
|
|
||
| $this->assertTrue(true); |
There was a problem hiding this comment.
Please assert for the annotations to be at least of the type you expected
| @@ -0,0 +1,22 @@ | |||
| <?php | |||
| /** | |||
|
|
||
| namespace Doctrine\Tests\Common\Annotations\Fixtures; | ||
|
|
||
| use \Doctrine\Tests\Common\Annotations\Fixtures\Annotation as Annotations; |
There was a problem hiding this comment.
Please add a comment explaining that the leading \ is intentional
| use \Doctrine\Tests\Common\Annotations\Fixtures\Annotation as Annotations; | ||
|
|
||
| /** | ||
| * Class ClassWithFullPathUseStatement |
|
|
||
| /** | ||
| * Class ClassWithFullPathUseStatement | ||
| * @package Doctrine\Tests\Common\Annotations\Fixtures |
| @@ -728,6 +728,11 @@ private function Annotation() | |||
| } | |||
| } | |||
|
|
|||
| // strip the first \ off if used | |||
There was a problem hiding this comment.
Can actually just call ltrim($name, '\\') all the time, and make this comment and conditional redundant.
|
@gnat42 the bug does not appear when running all tests, because you reused an annotation used in other tests, and so the class is already loaded when reaching your test. But the bug only happens when the class is not yet loaded (classes already loaded are detected using |
very clever! Indeed, please add that one :-) @stof he's already using |
|
@Ocramius I prefer using a separate annotation class used only for this test. Much less overhead than forcing a subprocess for the test, for the same result |
|
Agree, good point. The assertion to be added is obviously needed too. |
|
thus, I not even sure that |
|
even better than the assertion: if (class_exists(YourAnnotation::class, false)) {
throw new LogicException('The YourAnnotation must not be used in other tests for this test to be useful');
}We want to mark the test as an error when someone messes with the testsuite requirement, not as failed. The issue is not in the code but in the testsuite |
|
I think that I've addressed all changes/suggestions. Let me know if I missed any. |
|
@gnat42 Thanks |
|
Fixes #115 |
This PR demonstrates the issue with #115 as well as a potential fix.
There's one oddity here however, I can only cause the failure if I only run that one test. This is why the test I added has a '@group gnat' annotation.
When I run
Without my fix in place in DocParser it fails, running all tests for some reason doesn't fail. I'm not sure why. I also realize that really I should have a test against DocParser instead, since that is where the issue lies. However this was a proof of concept of the issue. If you comment out my changes in DocParser, the error we're getting in symfony based projects happens.