restrict reflection doc block #23848
Conversation
|
Can you expand the fixtures to make use of such an annotation? |
|
Are our tests broken because of this bug in phpdocumentor? If yes, we should merge (but a component's composer.json should be patched also, isn't it?), if not, it's not our job to provide an exclusion list to me. |
|
@nicolas-grekas Having an annotation without parentheses is so common that we IMO should not fail on those classes. |
|
then we need a test case that ensures we hit the issue also, as you asked for already btw :) |
|
Any hints on which test class to add an |
|
I added a simple testcase which definetly fails on 3.2.1. I required it localy and saw the test failing with the well know error. |
| @@ -72,6 +72,7 @@ public function typesProvider() | |||
| array('donotexist', null, null, null), | |||
| array('staticGetter', null, null, null), | |||
| array('staticSetter', null, null, null), | |||
| array('emptyVar', null, null, null), | |||
There was a problem hiding this comment.
looks like you need to adapt the ReflectionExtractorTest for this change
There was a problem hiding this comment.
This one was enough locally to get the issue. But i will have a look in the other one too.
There was a problem hiding this comment.
Now i understood what you ment :-)
|
Just give me a hint when i should squash the commits. |
| @@ -12,9 +12,12 @@ | |||
| namespace Symfony\Component\PropertyInfo\Tests\Fixtures; | |||
|
|
|||
| use Symfony\Component\Serializer\Annotation\Groups; | |||
| use Symfony\Component\Validator\Constraints as Assert; | |||
There was a problem hiding this comment.
this is wrong, was my first try. I will remove it.
|
|
||
| /** | ||
| * @author Kévin Dunglas <dunglas@gmail.com> | ||
| * |
There was a problem hiding this comment.
this too. This is not the annotation i whanted to test.
c0859fb to
9fc7723
Compare
| $this->assertTrue($this->extractor->isReadable('Symfony\Component\PropertyInfo\Tests\Fixtures\Dummy', 'id', array())); | ||
| $this->assertTrue($this->extractor->isReadable('Symfony\Component\PropertyInfo\Tests\Fixtures\Dummy', 'Guid', array())); | ||
| $this->assertFalse($this->extractor->isReadable('Symfony\Component\PropertyInfo\Tests\Fixtures\Dummy', 'guid', array())); | ||
| $this->assertEquals( |
There was a problem hiding this comment.
should be assertSame to match the previous assertions
The version 3.2.0 and 3.2.1 of reflection-docblock is broken and lower version as 3.1 miss some tags
9fc7723 to
6760127
Compare
|
|
|
Thank you @ElectricMaxxx. |
This PR was merged into the 3.3 branch. Discussion ---------- restrict reflection doc block | Q | A | ------------- | --- | Branch? | 3.1, 3.2, 3.3, master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT I think we should have same behavior on 2.8 and 3.0 but there `phpdocumentor/reflection` is required and i whanted to avoid conflicts. ### Reason: The version 3.2.0 and 3.2.1 of phpdocumentor/reflection-docblock is really broken. All Annotations lide `@api`, means without any bracket, leads to errors like `Uninitialized string offset: 0`. We in the CMF do not require that bundle directly, but symfony does and so we get braking builds. Commits ------- 6760127 restrict reflection doc block
I think we should have same behavior on 2.8 and 3.0 but there
phpdocumentor/reflectionis required and i whanted to avoid conflicts.Reason:
The version 3.2.0 and 3.2.1 of phpdocumentor/reflection-docblock is really broken. All Annotations lide
@api, means without any bracket, leads to errors likeUninitialized string offset: 0. We in the CMF do not require that bundle directly, but symfony does and so we get braking builds.