Move to a modern version of reflection docblock

[UndefinedOffset]

This pull request incorporates changes from #139 but more importantly switches to phpdocumentor/reflection-docblock ^5.2 to address compatibility with PHPUnit ^9.5 that Silverstripe 4.10 supports. I believe this may also break compatibility with Silverstripe projects using PHPUnit 5.7 because of conflicting dependencies between the two versions.

[UndefinedOffset]

@robbieaverill hate to be a pest on this but do you know who is active as a maintainer on this repo now? This pull is a bit of a blocker if you want to use PHPUnit 9 in your project with this module.

[robbieaverill]

Looks good, I think the version bump would either require a minor or major release in this module

[UndefinedOffset]

Ya I would think it's at least a minor bump, the two versions of phpdocumentor/reflection-docblock are pretty large gaps plus the base supported PHP version is also increased (more inline with modern Silverstripe).

[UndefinedOffset]

I've pushed some changes, see above for the one outstanding issue

[UndefinedOffset]

Note about \n on windows was added to the bottom of the readme in the "A word of caution" section.

[Firesphere]

LGTM now.
@robbieaverill Any thoughts?

I want to give it a few manual tests before merging and tagging a new release. I'm guessing this would be release line 4.x, given the updates.

[robbieaverill]

LGTM, it might be safest as a new major version release yeah.

[Firesphere]

A quick test shows all classes being prefixed with a \:

* @property string $Name
 * @property string $Lat
 * @property string $Lng
 * @method \DataList|\Location[] Locations()
 * @method \DataList|\Suburb[] Suburbs()
 */
class City extends DataObject

That does not sound correct to me

[Firesphere]

All @method tag classes are prefixed with a \, this causes the class paths to be wrong.

[Firesphere]

It looks like it might be here, something goes wrong and all classes are prefixed with a \

[UndefinedOffset]

@Firesphere this appears to be the behavior in the current version as well, and technically would make sense I think otherwise it would be relative to the current namespace of the file (at least if you tried to use that same path in PHP). VSCode using intellephence does seem to properly handle this, I'm curious how does your ide of choice handle it?

[Firesphere]

PHPStorm does not handle this properly, and I've not seen it do that before, as it shouldn't include the namespace, as far as I remember...

[UndefinedOffset]

Hmm I think that may cause more issues, particularly when it comes to aliased classes in the use statement. It looks like at least based on the PHPDocumentor docs that class names in types can be the FQCN or the local, but to do the local we'd need to parse the document I would think to figure out the alias if present as a use statement.

[Firesphere]

This has been discussed in a different PR, where we decided against both, IIRC.
Not adding the class as a use is the safer way to go.

[UndefinedOffset]

@Firesphere I think the issue you are seeing is because it's not resolving to the full path to the class. It really should be \SilverStripe\ORM\DataList not \DataList do you have the composer.json, composer.lock and sample code you used so I can see if I can re-produce what happened there?

[Firesphere]

Yes, it's literally above, you could probably determine my DB/Relations from the example there :)

[UndefinedOffset]

hmmm I didn't not observe that behavior using a model I have it generated as I would expect. Is it possible PHPStorm is adjusting them after this branch generates them?

/**
 * Class \Project\Model\Elements\CarouselElement
 *
 * @property int $SlideDelay
 * @property bool $DisableCarouselDots
 * @method \SilverStripe\ORM\DataList|\Project\Model\Elements\Children\CarouselElementItem[] Images()
 */

My local composer.lock has the following for PHPDocumentor's dependencies:

• phpdocumentor/reflection-common 2.2.0
• phpdocumentor/reflection-docblock 5.3.0
• phpdocumentor/type-resolver 1.6.1

Perhaps there is a miss-match in dependencies as well that maybe causing the problem?

[Firesphere]

Right, I see the problem, I think.

If the class is already in the use statements somewhere, it's being shorted, with the preceding \

[UndefinedOffset]

Ah do you have the SilverLeague\IDEAnnotator\DataObjectAnnotator.use_short_name config option set to true by chance?

[UndefinedOffset]

If so I believe that's the source of the issue, I'm not sure that will work properly with the newer versions of phpDocumentor. There maybe something with the context that can get passed into $this->tagFactory->create() but I'm not 100% sure how that works. If I pass a context created from the ReflectionClass instance it tries to resolve but it gets the path wrong.

It makes me wonder if we should just remove that config option for this new version? It sounds like PHPStorm and most ides (from a quick google) should properly support the FQNs.

[Firesphere]

We can't remove that option, sadly/probably.

Reason is, that if people go in to the file, and import the class with a use case, the IDE like PHPStorm, will shorten it.

A build will then lengthen it again, or break it. Neither of those is a desirable outcome.

[UndefinedOffset]

@Firesphere sorry for the slow follow up I've added a new resolver that seems to address the issue you called out. Let me know 😄

[Firesphere]

While at it, could you please update the following?
AbstractTagGenerator.php::116
return (array)call_user_func_array('array_merge', array_values($tags));

This will make it PHP 8.1 compatible in a single go

[Firesphere]

I have no other comments, but would like that minor change to be added in this PR and make it all go to a new point release

[UndefinedOffset]

I've replaced the line you referenced with your line @Firesphere (with $this->tags instead of $tags 😉)