Exporting tags

[barryvdh]

The export functions are not yet implemented, because it says it has to see what reflection outputs. Doesn't the getDocComment function just return the comments, as found in the php file?

What are you planning to do with the export, let the tags return the line without the * (so just @param int $i for example), and let the docblock itself just loop through the tags and call the export function, and place the lines in the docblok comment style?

Are you planning on implementing that, or would you like to let me make a PR?

[mvriel]

At current there are no actual plans for that feature, I'd like it if it were to mimic the style of the PHP Reflection library but this item has a lower priority than other features and bugs at the moment.

A PR is much appreciated as that would strengthen this component a lot and I would be very grateful.

An example of the style of this output is: http://stackoverflow.com/questions/6274195/format-of-exported-reflection-class-in-php

[boenrobot]

It's more about what is the purpose and style of export in PHP's native Reflection API, and then making ReflectionDocBlock align with that purpose and style, as opposed to potentially hindering it. That is pretty much an unknown at this point... I mean, why would anyone call export() on anything ever?

Also, there's the __toString() method... how should that fit into the picture? How does it fit in the Reflection API?

We do plan on eventually adding serialization abilities to ReflectionDocBlock, so that you can generate a doc comment out of a DocBlock object and/or a string representing a tag out of a tag object. Note that the latter is easily achievable today by simply calling getName() and getContent() methods.

[barryvdh]

Okay, I was more interested in the latter, exporting a doc comment out of a docblock object.

Calling getName and getContent is what I do now, but now I have to loop through the tags and create the docblock output myself. I thought that this library would be a more logical place to do that.

But what I'm trying to do also requires the ability of changing just 1 part, to do something like this:

1. Load docblock from method using reflection (is working)
2. Add a tag (is working now)
3. Change the return type (can be done using setContent('type description'), but I'd rather do just setType('type')
4. Generate a new docblock comment (not possible with this library yet).

So how do you feel about generating the comment? I could try to make a PR for that (and perhaps some setType() functions for some tags).
I'm not really sure what to make of the export() output as mentioned in StackOverflow, because I don't know what you should do with it.

[barryvdh]

I created a PR to make it possible what I just said. Please let me know what you think about it, or if I need to make some changes.

[boenrobot]

Yeah, about the setType() thing... (@mvriel we have to have that discussion again it seems, this time openly...)

Here's the thing - at one point, we thought about moving the type stuff into the Collection object, where you could modify stuff. However, as I figured out as soon as I actually tried to do it... turns out if we do that, the serialization - or rather, the getContent() method - becomes impossible to cache. Currently, if you call getContent() once, the next time you call it, it will pull up a pre-computed string. Thus, if you serialize the same tag multiple times during a script (e.g. if you want to serialize "before" and "after"), it will be faster now than with such modifications.

For the getContent() method to be cachable, either the methods modifying type need to be within the Tag object, or the Collection object must somehow notify the Tag object about the modifications, so that the Tag object can clear the cache (or not, if no such notifications arrive).

And there's also a thing about the naming... I mean just look at the inconsistency between the properties and their respective methods in the source... @mvriel this wouldn't be the first time I say "come on, let's fix this, pleeeease... we can easily fix phpDocumentor accordingly". Especially now where it explicitly requires tags and all that... Just release b1, and let's have that for b2. Let's have getType() and setType() deal with raw types, and have getTypes() output a parsed collection (regenerated upon raw modifications) and maybe getParsedType() that would output what getType() currently does. Sure, it's not as ideal as modifying individual type components, but it's still better than nothing, right?

@barryvdh If you're going to make a pull about the serializer, please make it a separate class that will maintain serialization options. This will ensure serialization does not cause larger memory overhead for read-only consumers, such as phpDocumentor2, and will also enable easier reuse of said options, which I'd think is a pretty common case. What serialization options? E.g. number of spaces before the "*" on each line, line length limit (to wrap content when its over that), and more I can't think of right now.

EDIT: Saw the pull request... I like (or should I say "don't mind") the getDescription()/setDescription() duo on the DocBlock object, but you can see my remarks about the rest.

[mvriel]

Yeah, about the setType() thing... (@mvriel we have to have that discussion again it seems, this time openly...)

I do not directly recall this discussion; might be because the context here does not trigger my memory or that I have been discussing too much ;)

Here's the thing - at one point, we thought about moving the type stuff into the Collection object, where you could modify stuff. However, as I figured out as soon as I actually tried to do it... turns out if we do that, the serialization - or rather, the getContent() method - becomes impossible to cache. Currently, if you call getContent() once, the next time you call it, it will pull up a pre-computed string. Thus, if you serialize the same tag multiple times during a script (e.g. if you want to serialize "before" and "after"), it will be faster now than with such modifications.

For the getContent() method to be cachable, either the methods modifying type need to be within the Tag object, or the Collection object must somehow notify the Tag object about the modifications, so that the Tag object can clear the cache (or not, if no such notifications arrive).

I am not sure I get what this is about; a legacy point of ReflectionDocBlock is that in the past it only supported 1 type on an element. Later it became obvious this was false and a secondary method of types was introduced to maintain backwards compatibility.

Can you elaborate on this?

And there's also a thing about the naming... I mean just look at the inconsistency between the properties and their respective methods in the source... @mvriel this wouldn't be the first time I say "come on, let's fix this, pleeeease... we can easily fix phpDocumentor accordingly". Especially now where it explicitly requires tags and all that... Just release b1, and let's have that for b2. Let's have getType() and setType() deal with raw types, and have getTypes() output a parsed collection (regenerated upon raw modifications) and maybe getParsedType() that would output what getType() currently does. Sure, it's not as ideal as modifying individual type components, but it's still better than nothing, right?

I believe we should ditch the raw content and raw typing thing; I can't think of a reason to maintain two copies of the same information.

Can you name some of those inconsistencies so I have a feeling what you are getting at; I am not sure I understand.

[barryvdh]

About the getType(), I'm not sure about the technical difficulties behind it, but it seems a bit strange that the other Tags have methods for using a string to set parts, but return tags don't.

I will see if I can create seperate Serializer class.

[mvriel]

None of the tags are capable of setting a Type using a string; the Type(s) is/are represented by a Collection object

[barryvdh]

No that's what I meant, you have setAutherName, setVariableName, setArguments etc which all can be set with a string, only the type cannot be set.

[mvriel]

That's because Type is not one thing; Type is a complex expression that may be interpreted in several ways.
For that matter; Type is not accurate as it may contain multiple types or even array definitions

[barryvdh]

I agree the name might not be very accurate, but if you use getType to return a string representation, I don't see the problem in also using setType() (or you should change getType to, but that would break compatability I guess..)
Or what would you suggest, how should the type/types be set?

[mvriel]

My inclination would be to drop the setType/getType methods, thus remove any reference to strings and completely switch to the Collection object.

[boenrobot]

I do not directly recall this discussion; might be because the context here does not trigger my memory or that I have been discussing too much ;)

It was pre-"Big Refactoring™" IRC discussion, so I don't blame you for not recalling.

EDIT: Well, took me long enough to write this... our conversation back then basically went similar to how the conversation above went.

Can you elaborate on this?

Currently, there is a separate object containing type information (Type\Collection). Imagine you are allowed to modify that collection, and that you do indeed modify it. Then, you call the according tag's getContent() method.

Here's the problem - if you've modified a separate object, how is the Tag object supposed to know that this separate object was modified? It can't, so every time you call getContent(), the only way to get the right result is to unconditionally get the parsed string representation (what getType() currently does). So, every time (not just the first time!) you call getContent(), you have the overhead of getType(), plus, you can't get "raw" content even if you need it.

Contrast that with what would happen if you can't modify that collection, but you can modify the "type" property (the "raw" component) - since it's within the Tag object, whenever you call getContent(), the content can be "cached" within the "content" property, and this cache can be "cleared" (set to NULL) when the "type" property is modified. That way, getContent() only has an overhead the first time (per modification) you call it, rather than every time.

I believe we should ditch the raw content and raw typing thing; I can't think of a reason to maintain two copies of the same information.

The very scenario this issue discusses is one where separation is needed. The scenario about generating or modifying doc blocks. When you do that, you don't want stuff modified unless you've asked for it. If only the parsed components are kept, there's no way to serialize them into "raw" components.

Can you name some of those inconsistencies so I have a feeling what you are getting at; I am not sure I understand.

The "types" property is returned as a string by getType() (as opposed to getTypes()), getTypesCollection() gets the "types" property (what you'd think a getTypes() method would do), while getTypes() returns "types" as an array (which is fine from a consumer point of view, but internally, it doesn't match). All while the "type" property is not being gettable/settable with anything right now, and (what would be) the consistent getType() is already "taken".

[barryvdh]

Okay, I left out the setType part, so that is for you two to discuss ;)
I also moved it to a seperate class, with the suggestions you made.

[barryvdh]

Just to be clear, currently the only way to change the return type(s), is to do something like $tag->setContent($newType .' '.$tag->getDescription()), right?

[boenrobot]

Yes... far from ideal, I know.

[barryvdh]

Well then I still fill that something like this wouldn't be weird, but it is your call ;)
You could maybe just name it setRawType of setRawTypes or setTypesFromString..
public function setType($type){ $this->type = $type; $this->types= null; $this->content = null; return $this; }

And in setContent(), shouldn't $this->types be set to null? Now when setContent is called after getTypesCollection, and then getTypesCollection again, it doesn't recreate the collection. Or am I overlooking something?

Thanks anyways for merging this!

[boenrobot]

setRawType() was actually how me and @mvriel ended the last conversation 😆 .

It's fine... but it's still inconsistent IMHO, which is why I never got around to it. I thought "we'll talk tomorrow about the inconsistency thing, and hopefully I'll do it right rather than patchy like that", and that conversation happened instead just now LOL.

And in setContent(), shouldn't $this->types be set to null?

Yep. Good catch. I'll fix that right now.

[barryvdh]

Actually, when you read the documentation for @param and @return (http://www.phpdoc.org/docs/latest/for-users/phpdoc/tags/return.html), they always speak about a Type.
Syntax: @return [Type] [<description>]
And a Type can consist about multiple (sub)types.
When the Type consists of multiple (sub-)types then these MUST be separated with the vertical bar sign (|).

So getType() may not be consistent because you return multiple things, but when reading the phpDocumentor docs, it would make sense to expect it to be called Type.
So imho it would make to most sense to have the following cases:

$tag->setType('bool|null');
$tag->setTypes(array('bool', 'null');
$tag->addType('string');

$tag->getType();    //returns bool|null
$tag->getTypes()    //returns array('bool', 'null');

And one more suggestion, if you don't plan to use the export() function as __toString(), you could think about making the Description class return getContents() in its __toString(), so both getShortDescription() and getLongDescription() can be directly outputted as a string. (And for tags, it wouldn't be bad to output the serialized version, so @param bool True when x, which would make instantly clear what the tag contains..)

[boenrobot]

I totally agree about the types thing (well, maybe except "addType"; sounds like overkill if you're allowed to get and set an array of types), and that's exactly what's not happening currently.

Keep in mind that the real problem we're discussing above is not about scalar types, but namespaced types - should we return them "as written" (raw) or "fully qualified, regardless of how they're written" (parsed). I argue we should have methods for both, with getType() dealing with the raw.

making the Description class return getContents() in its __toString(), so both getShortDescription() and getLongDescription() can be directly outputted as a string. (And for tags, it wouldn't be bad to output the serialized version, so @param bool True when x, which would make instantly clear what the tag contains..)

Fine by me. @mvriel ?

[barryvdh]

I think you do need both, but on the default you would probably want to have the parsed values (I would think that anyways).
The way it was, was that getType() retuned the parsed types, and getContent() the raw value.
Anyways, submitted two PR's, see what you do with them ;)