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

Jump to bottom

[PropertyInfo] Make PhpDocExtractor compatible with phpDocumentor v5 #36975

Merged
merged 1 commit into from Jun 15, 2020
Merged

[PropertyInfo] Make PhpDocExtractor compatible with phpDocumentor v5 #36975

merged 1 commit into from Jun 15, 2020

Conversation

DerManoMann
Copy link
Contributor

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #36049
License MIT
Doc PR N/A

Version 5 of phpDocumentor introduced some changes to the getTagsByName() method that break the PhpDocExtractor.

More specific, it now returns an instance of InvalidTag instead of null when parsing an invalid tag.

@DerManoMann DerManoMann requested a review from dunglas as a code owner May 26, 2020 23:52
@nicolas-grekas nicolas-grekas added this to the 4.4 milestone May 28, 2020
@nicolas-grekas
Copy link
Member

Can you please check if this change applies to branch 3.4, and rebase+retarget if yes?

@DerManoMann
Copy link
Contributor Author

Can you please check if this change applies to branch 3.4, and rebase+retarget if yes?

@nicolas-grekas I think it does.

I also just found #36850 which makes this a duplicate - is it worth progressing this one too?

One major problem both PRs are facing is the lowest/highest dep testing as it means we end up with different behaviour depending on used version of phpDocumentor.

Is there a recipe for how to deal with that? Conditional tests sounds a bit ugly to me, although detecting the new InvalidTag class as conditional would make sense...

@nicolas-grekas
Copy link
Member

Conditional testing is the way to go yes.
Can you please compare with #36850 and decide which PR should be moved forward if there are significant differences?

@DerManoMann
Copy link
Contributor Author

DerManoMann commented Jun 10, 2020 •
edited

@nicolas-grekas I know I said this applies to the 3.4 branch too, but I think things are more complicated.

On 3.4 tests fails straight off due to BC breaking changes, for example detection of nullable. That is unrelated this issue and I will require bigger changes, IMHO.

As far as this PR against 4.4 is concerned - I've made the tests conditional but struggle to find the right way to test for the new code. For some reason the PHP 7.1 tests on travis use phpdocumentor 5.0.0-alpha5. However, the new InvalidTag class was only introduced in alpha8...
EDIT: I worked around this issue by detecting return type hints at runtime... alpha5 is the latest usable for PHP 7.1 as after that the requirements got bumped to PHP 7.2...

Finally, there doesn't seem to be any progress on #36850, so I am happy to continue work on this PR if I could get some advice/guidance on the issues described above.

@DerManoMann
Copy link
Contributor Author

@nicolas-grekas @stof not sure what the etiquette rules are here, but this needs a re-review and a call on whether 4.4 is the right place for it (it is, IMHO, as outlined in the previous comment)

nicolas-grekas
nicolas-grekas approved these changes Jun 14, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(with minor comments)

src/Symfony/Component/PropertyInfo/Tests/Extractor/PhpDocExtractorTest.php Outdated Show resolved Hide resolved
src/Symfony/Component/PropertyInfo/Tests/Extractor/PhpDocExtractorTest.php Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas mentioned this pull request Jun 14, 2020
Closed
4 tasks
@DerManoMann
Make PhpDocExtractor compatible with phpDocumentor v5
b1f8e5a 
Version 5 of phpDocumentor introduced some changes to the
`getTagsByName()` method that break the `PhpDocExtractor`.
More specific, it now returns an instance of `InvalidTag` instead of
`null` when parsing an invalid tag.
@fabpot
Copy link
Member

fabpot commented Jun 15, 2020

Thank you @DerManoMann.

@fabpot fabpot merged commit bb8e66b into symfony:4.4 Jun 15, 2020
@fabpot fabpot mentioned this pull request Jun 15, 2020
Merged
This was referenced Jul 24, 2020
Merged
Merged
@derrabus derrabus mentioned this pull request Aug 10, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@stof stof stof approved these changes

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

Assignees
No one assigned
Labels
Bug PropertyInfo Ready Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@DerManoMann @nicolas-grekas @fabpot @stof @carsonbot