[FrameworkBundle] Execute the PhpDocExtractor earlier #21370

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@GuilhemN
Contributor
Q A
Branch? master
Bug fix? yes, but safer to apply on master
New feature? no
BC breaks? is changing a priority a bc break?
Deprecations? no
Tests pass? yes
Fixed tickets #21367
License MIT
Doc PR

Fixes #21367.

I wonder if this is logical to execute the PhpDocExtractor after the ReflectionExtractor: when you use phpdocs it's because they are more precise than php type hints.
This causes an issue in NelmioApiDocBundle, for example you can't use int[] with a setter as the type mixed[] will be returned instead of int[].

Would you accept bumping its priority to -999?

This PR changes the priority of the ReflectionExtractor to -1002 to make sure it is executed after the PhpDocExtractor.

@nicolas-grekas
Member

Not sure it should go on master to me. To reviewers: please advise your pov :)

@GuilhemN GuilhemN [FrameworkBundle] Execute the PhpDocExtractor earlier
fd8ab6c
@GuilhemN
Contributor

@nicolas-grekas do you think it should go in 2.8? It is fine for me either way.

@xabbuh
Member
xabbuh commented Jan 30, 2017

Looks like a bug fix to me.

@fabpot
Member
fabpot commented Jan 30, 2017

@dunglas's comment on the issue is interesting; it was done for performance reasons. This change makes sense but can we check that performance is still ok?

@GuilhemN
Contributor
GuilhemN commented Jan 30, 2017 edited

@fabpot that's why there is a cache layer, i don't think it's relevant.

@fabpot
Member
fabpot commented Jan 30, 2017

Thank you @GuilhemN.

@fabpot fabpot added a commit that referenced this pull request Jan 30, 2017
@fabpot fabpot bug #21370 [FrameworkBundle] Execute the PhpDocExtractor earlier (Gui…
…lhemN)

This PR was submitted for the master branch but it was merged into the 2.8 branch instead (closes #21370).

Discussion
----------

[FrameworkBundle] Execute the PhpDocExtractor earlier

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes, but safer to apply on master
| New feature?  | no
| BC breaks?    | is changing a priority a bc break?
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #21367
| License       | MIT
| Doc PR        |

Fixes #21367.

> I wonder if this is logical to execute the PhpDocExtractor after the ReflectionExtractor: when you use phpdocs it's because they are more precise than php type hints.
This causes an issue in NelmioApiDocBundle, for example you can't use int[] with a setter as the type mixed[] will be returned instead of int[].
>
> ~~Would you accept bumping its priority to -999?~~

This PR changes the priority of the `ReflectionExtractor` to `-1002` to make sure it is executed after the `PhpDocExtractor`.

Commits
-------

0425e05 [FrameworkBundle] Execute the PhpDocExtractor earlier
f4693be
@fabpot fabpot closed this Jan 30, 2017
@GuilhemN GuilhemN deleted the GuilhemN:INTARRAY branch Jan 31, 2017
@fabpot fabpot added a commit that referenced this pull request Jan 31, 2017
@fabpot fabpot minor #21482 [FrameworkBundle] Fix tests (dunglas)
This PR was merged into the 3.3-dev branch.

Discussion
----------

[FrameworkBundle] Fix tests

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Travis is red because of the new test introduced in #21370. This should make it green again.

Commits
-------

1bf451c [FrameworkBundle] Fix tests
b3b3dac
@nicolas-grekas nicolas-grekas added a commit that referenced this pull request Feb 1, 2017
@nicolas-grekas nicolas-grekas minor #21486 [FrameworkBundle] add missing functional Serializer test…
… case (xabbuh)

This PR was merged into the 2.8 branch.

Discussion
----------

[FrameworkBundle] add missing functional Serializer test case

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

This is needed to make the test introduced in #21370. It basically backports the functional test config as introduced by @dunglas in #20480.

Commits
-------

24243ac add missing functional Serializer test case
6316f34
This was referenced Feb 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment