BetterReflection icon indicating copy to clipboard operation
BetterReflection copied to clipboard

Remove @internal from LocatedSource.php

Open rodrigopedra opened this issue 2 years ago • 3 comments

While Roave\BetterReflection\SourceLocator\Type\AbstractSourceLocator is not marked as @internal, it expects a child class to implement the createLocatedSource() method.

In turn, the createLocatedSource() method has a return type of Roave\BetterReflection\SourceLocator\Located\LocatedSource|null, and Roave\BetterReflection\SourceLocator\Located\LocatedSource which is marked as @internal.

This PR

  • Removes the @internal from Roave\BetterReflection\SourceLocator\Located\LocatedSource's docblock

This is more like an annoyance, when writing custom source locators that extend from Roave\BetterReflection\SourceLocator\Type\AbstractSourceLocator, than an issue, as static analysis flags one is using an internal class.

Feel free to reject this PR, or, maybe, mark Roave\BetterReflection\SourceLocator\Type\AbstractSourceLocator as @internal, in case this change is not desirable.

rodrigopedra avatar Oct 30 '23 04:10 rodrigopedra

Just in case, the Roave\BetterReflection\SourceLocator\Type\SourceLocator interface expects an implementing class to implement its locateIdentifier() method, which, in turn, has a return type of Roave\BetterReflection\Reflection\Reflection|null.

Roave\BetterReflection\Reflection\Reflection is also marked as @internal.

rodrigopedra avatar Oct 30 '23 04:10 rodrigopedra

If we want to make this public, perhaps an interface would be better 🤔

Ocramius avatar Oct 30 '23 08:10 Ocramius

If we want to make this public, perhaps an interface would be better 🤔

I'd originally marked it @internal as it's a bit of a detail; it was originally a value object of sorts, but wasn't sure about the best public API for it.

That said, I understand the OP concern; I noticed the same in fact when consuming this in Roave/BackwardCompatibilityChecker - not actually sure why Psalm hasn't flagged that there, possibly because the top level Roave namespace is the same.

Given that LocatedSource is indeed returned and used by various public API in Better Reflection, and the library is much more mature now, I personally wouldn't be opposed to adding this as part of the public API (i.e. removing @internal, as proposed).

asgrim avatar Oct 30 '23 08:10 asgrim