code-lts / doctum

A php API documentation generator, fork of Sami
https://doctum.long-term.support/
MIT License
300 stars 32 forks source link

Class property hints do not resolve aliases #23

Closed CHItA closed 3 years ago

CHItA commented 3 years ago

Type aliases are not resolved for class properties. E.g. the following code snipet would not generate the expected some\namespace\someClass type hint in the docs:

use some\namespace\someClass;
class Foo {
/** @var someClass */
protected $someClass;
}

This seems to be the case for the current version of doctum as well. Would you consider this a bug or is this something doctum doesn't and will not support?

williamdes commented 3 years ago

Hi, This looks like a bug to me, I will try to have a look and fix this as soon as possible

Thank you for reporting it !

CHItA commented 3 years ago

Thanks! By the way, if this is not a trivial fix for you either, I'd be happy to send patch, I'm just highly unfamiliar with the code base, and wasn't sure how docblocks supposed to be parsed. So feel free to let me know if you need help with this.

williamdes commented 3 years ago

Thanks! By the way, if this is not a trivial fix for you either, I'd be happy to send patch, I'm just highly unfamiliar with the code base, and wasn't sure how docblocks supposed to be parsed. So feel free to let me know if you need help with this.

I think that a fix would be the best idea to speed this up but I am not familliar either with all the codebase, the best thing is to make a patch but writing a test for it until it passes. This adds coverage and avoids BC breaks

One thing you can do is add a test case (create a folder with a file that has your example and add a sample config file) in the actual test suite and then debug the code until you find where the issue is.

Let me know if you are working on it even if afterwards you abandon :)

CHItA commented 3 years ago

Sure, I can do that. I think I should be able to take a look at this in the next couple of days.

CHItA commented 3 years ago

I tinkered with this a bit yesterday, and it doesn't seem like a trivial fix. As far as I can tell this is the change that introduced this bug, however, just adding phpDocumentor\Reflection\Types\Context back does not fix the problem.

This is because the use statement handling is broken as well. For example,

use Foo\Bar\A;
use Foo\Baz\B;

will produce the following alias array: ['' => 'Foo\Baz\B'], while phpDocumentor (and I) would like to see ['A' => 'Foo\Bar\A', 'B' => 'Foo\Baz\B']. Probably.

A third thing is that the DocBlockParser::parse method is always called with 3 arguments, where it never took more than two. This is like that since Sami exists as far as I can tell, so not sure if there is a reason for that or not.

williamdes commented 3 years ago

I will keep this one open until I release the final version :)

williamdes commented 3 years ago

@CHItA Your changes work great ! I just tested them on https://github.com/code-lts/Laravel-FCM

Would you mind running the latest -dev phar and let me know if everything still works good ? (I did some code improvements that could create unexpected type errors)

CHItA commented 3 years ago

@williamdes https://doctum.long-term.support/releases/dev/doctum.phar doesn't seem to have your latest commits. Is there a build somewhere I could use?

CHItA commented 3 years ago

With master it seems to work fine.

williamdes commented 3 years ago

With master it seems to work fine.

I updated the PHAR file for dev, should be alright now Let me know if it is not up to date I will add some way to know more about the phar build commit, date, etc..

williamdes commented 3 years ago

Latest Phar I just pushed enables doctum.phar version --text

CHItA commented 3 years ago

Latest .phar gives me the same results as the main branch did. So it looks good to me.

williamdes commented 3 years ago

I released your changes 🚀 https://github.com/code-lts/doctum/releases/tag/v5.4.0