Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove @internal from LocatedSource.php #1373

Open
wants to merge 1 commit into
base: 6.16.x
Choose a base branch
from

Conversation

rodrigopedra
Copy link

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
Copy link
Author

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.

@Ocramius
Copy link
Member

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

@asgrim
Copy link
Member

asgrim commented Oct 30, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants