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

Upgrade to reflection-docblock 3 #263

Closed
wants to merge 2 commits into from

Conversation

dunglas
Copy link
Contributor

@dunglas dunglas commented Mar 1, 2016

Tests will be green when phpDocumentor/TypeResolver#16 and phpDocumentor/ReflectionDocBlock#72 will be merged.

@dunglas dunglas force-pushed the reflection-dockbloc-3 branch 2 times, most recently from fc44c53 to 3127866 Compare March 1, 2016 13:22
@dunglas dunglas force-pushed the reflection-dockbloc-3 branch from 3127866 to bb533ff Compare March 1, 2016 13:23
/**
* @var Method[] $tagList
*/
$tagList = isset($phpdoc) ? $phpdoc->getTagsByName('method') : array();
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the idea of dealing with codepath where the variable may be defined or no. The code should probably be refactored by moving this inside the try/catch

@stof
Copy link
Member

stof commented Mar 1, 2016

This PR means that the min requirement of Prophecy is now PHP 5.5 though. and given that PhpSpec still need to support PHP 5.3+ until June, this is not acceptable.

@dunglas
Copy link
Contributor Author

dunglas commented Mar 1, 2016

But currently it means that Prophecy is not usable with Symfony master. As SF 3.1 will be released in May, maybe can we wait June to merge this PR? An alternative is to allow both versions and execute a different logic depending of the installed dependency version.

@aik099
Copy link
Member

aik099 commented Mar 1, 2016

So Symfony wants Reflection v3, but Prophecy wants Reflection v2?

@sroze
Copy link

sroze commented Mar 16, 2016

We need to be able to support the two versions, else it will again cause a lot of problems. Based on the small amount of changes that are required, that's fair to abstract it and uses a basic class_exists to create the correct adapter here IMO.

@dunglas
Copy link
Contributor Author

dunglas commented Mar 16, 2016

I fully agree, will do it ASAP.

@dunglas
Copy link
Contributor Author

dunglas commented Mar 25, 2016

Closing in favor of #264

@dunglas dunglas closed this Mar 25, 2016
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