Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

DocBlock Reflection not returning correct tags #5043

Closed
wants to merge 22 commits into from
Closed

DocBlock Reflection not returning correct tags #5043

wants to merge 22 commits into from

Conversation

danez
Copy link
Contributor

@danez danez commented Aug 28, 2013

Currently when using reflection on a phpDoc the tags returned are not as expected. For example for an @return tag i expect Zend\Code\Generator\DocBlock\Tag\ReturnTag to be returned, but instead Zend\Code\Generator\DocBlock\Tag is always returned (which is missing the types-property from the reflection tag).

When working on a fix i encountered, that the whole generator tags topic does not work with reflection at all. e.g. AuthorTag throws an Exception about undefined function. And the nameing inside the Zend\Code\Generator\DocBlock\Tag* is completely off.

I would like to do a little rewrite, without breaking BC:

  • New renamed properties and getter and setters in Author-, License-, Return-, ParamTag (with BC except Author which is completely broken)
  • Make fromReflection() deprecated from these classes (in favor of TagManager)
  • Add new classes for other Tags similar to the Reflection namespace
  • Move Zend\Code\Generator\DocBlock\Tag to class GenericTag
  • Create a TagManager similar to Reflection namespace (these class decides which class to create)
  • Change DocBlockGenerator to use TagManager
  • Create Tests for new TagClasses and TagManager

What do you think about that?

@danez
Copy link
Contributor Author

danez commented Aug 29, 2013

I tried to maintain backward compatibility were I could, the only exceptions are:

  • Zend\Code\Generator\DocBlock\Tag\AuthorTag: removed set/getDatatype() and set/getParamName()
  • Zend\Code\Generator\DocBlock\Tag\AuthorTag: __construct changed from ($options = array()) to ($authorName = null, $authorEmail = null)
  • Zend\Code\Generator\DocBlock\Tag\LicenseTag: __construct changed from ($options = array()) to ($url = null, $licenseName = null)
  • Zend\Code\Generator\DocBlock\Tag\ReturnTag: __construct changed from ($options = array()) to ($types = array(), $description = null)
  • Zend\Code\Generator\DocBlock\Tag\ParamTag: __construct changed from ($options = array()) to ($variableName = null, $types = array(), $description = null)
  • Using DocBlockGenerator::fromReflection() and afterwards getTags() is now returning the new Tag classes (ReturnTag, AuthorTag, ParamTag, ...) where applicable and otherwise GenericTag. The deprecated class Tag will not be returned anymore.

{
$output = '@method'
. (($this->isStatic) ? ' static' : '')
. ((!empty($this->types)) ? ' ' . implode('|', $this->types) : '')
Copy link
Member

Choose a reason for hiding this comment

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

May you can move the implode(...) part to the parent class in some method like typesToString

@Maks3w
Copy link
Member

Maks3w commented Aug 29, 2013

@danez The PR conflicts with some code in master. Can you rebase it?

@Maks3w
Copy link
Member

Maks3w commented Aug 29, 2013

@ralphschindler May you want to review this.

@danez
Copy link
Contributor Author

danez commented Aug 29, 2013

Rebased on master and moved the imploding to abstract class.

Daniel Tschinder added 5 commits August 29, 2013 14:27
Also allow is*() methods parallel to set*() methods in TagManager like isStatic()
remove one redundant comment
test PropertyClassFactory
@danez
Copy link
Contributor Author

danez commented Aug 29, 2013

I think I tested all the new parts and should be ready for review

@Maks3w
Copy link
Member

Maks3w commented Aug 29, 2013

Could you add class docblocks to all new classes describing and explaining the purpose of the class?

* @link http://github.com/zendframework/zf2 for the canonical source repository
* @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
* @license http://framework.zend.com/license/new-bsd New BSD License
* @package Zend_Code
Copy link
Contributor

Choose a reason for hiding this comment

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

remove @Package

@danez
Copy link
Contributor Author

danez commented Aug 29, 2013

I added class descriptions, all other classes + namespace are selfexplanatory.
According to the comments on the headers I removed @Package from all file headers in the test files.
Also I removed @category, @Package, @subpackage, @license and @copyright from class comments in the test files.

@danez
Copy link
Contributor Author

danez commented Aug 29, 2013

The deprecated comments may need some version in them, but I'm not sure what to put there. 2.2.x? 2.3.0?

@weierophinney
Copy link
Member

The deprecated comments may need some version in them, but I'm not sure what to put there. 2.2.x? 2.3.0?

2.3.0 -- this is a substantial change, so I'll target it for the next minor version. That said, the bits I saw were mostly BC, and the bits that weren't really didn't matter as you are not expected to create new instances manually for docblock tags (they should be created as part of reflection on their parents!).

Add the deprecation comments, and I'll do final review -- and thanks!

@danez
Copy link
Contributor Author

danez commented Sep 3, 2013

Sometimes you might do and instantiate tags when generating code without reflection. But yeah most of the time you do not.
All the BC changes are up in the first comment.

weierophinney added a commit that referenced this pull request Sep 3, 2013
DocBlock Reflection not returning correct tags
weierophinney added a commit that referenced this pull request Sep 3, 2013
weierophinney added a commit that referenced this pull request Sep 3, 2013
@ghost ghost assigned weierophinney Sep 3, 2013
@weierophinney
Copy link
Member

Merged to develop for release with 2.3.0.

@danez
Copy link
Contributor Author

danez commented Sep 3, 2013

thanks

@danez danez deleted the fixDocBlockReflection branch September 3, 2013 16:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants