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

Reflection dockbloc 3 #264

Closed
wants to merge 6 commits into from

Conversation

theofidry
Copy link
Contributor

Follow up of #263:

  • Adapted the code to be able to run in both v3 and v2 of phpdocumentor/reflection-docblock
  • Added a Travis build with phpdocumentor/reflection-docblock 3.x

cc @dunglas.

private function getClassTagList(\ReflectionClass $reflectionClass)
{
try {
$phpdoc = (null === $this->docBlockFactory || null === $this->contextFactory)
Copy link
Member

Choose a reason for hiding this comment

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

That doesn't look like proposed adapter creation. This IF statement checking which Reflection library version is being used is called each time, which is bad performance wise.

Copy link
Contributor

Choose a reason for hiding this comment

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

A if statement is nothing compared to all parsing operations involved...

Copy link

Choose a reason for hiding this comment

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

The main point is that it's not an adapter at all :)

@theofidry
Copy link
Contributor Author

@aik099 I went with the simplest solution at hand. But if you prefer I can add a true adapter to retrieve tags and let you guys choose which one you prefer.

@theofidry
Copy link
Contributor Author

@aik099 here you go

final class ClassAndInterfaceTagRetriever implements MethodTagRetrieverInterface
{
/**
* @var MethodTagRetrieverInterface

Choose a reason for hiding this comment

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

To be consistent with property $tagRetriever on class MagicCallPatch, you should add PHPDoc everywhere or nowhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this one should be removed

@dunglas
Copy link
Contributor

dunglas commented Mar 24, 2016

phpDocumentor/TypeResolver#16 has been merged and a new version tagged. Tests should pass now, @theofidry can you add a conflict section in composer.json for Type Resolver < 0.1.7?

@theofidry theofidry force-pushed the reflection-dockbloc-3 branch from 83be274 to 4a97e8b Compare March 25, 2016 18:39
@everzet
Copy link
Member

everzet commented Apr 7, 2016

Still failing

@theofidry
Copy link
Contributor Author

@everzet there is something really weird on docblock side so needs to debug it :/ Maybe I'll have time to check it next week

@everzet
Copy link
Member

everzet commented May 9, 2016

I'm closing this PR as it is seems to be abandoned. Please feel free to rebase and resubmit it if that's not the case!

@linaori
Copy link

linaori commented May 30, 2016

As the issue is still valid, any chance this will be re-opened or should a new PR be made? Symfony 3.1 is released and this dependency will prevent people from upgrading.

@stof
Copy link
Member

stof commented May 30, 2016

Well, if @theofidry wants to finish it, I can reopen it. If you want to take it over to finish it instead, you should fetch the branch from @theofidry's fork and then create a new PR with the updated version.

@dunglas
Copy link
Contributor

dunglas commented May 30, 2016

I'm working on this one right now.

@GrahamCampbell
Copy link
Contributor

@dunglas 👍

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.

9 participants