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

Removed doc blocks parsing support #900

Merged
merged 3 commits into from
Dec 5, 2021
Merged

Conversation

kukulich
Copy link
Collaborator

@kukulich kukulich commented Dec 4, 2021

Fixes #899

src/Reflection/Annotation/AnnotationHelper.php Outdated Show resolved Hide resolved
composer.lock Outdated Show resolved Hide resolved

private static function getDocBlockFactory(): DocBlockFactory
{
self::$docBlockFactory ??= DocBlockFactory::createInstance();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As previously discussed in #899, I'm fully on board with dropping type parsing: what's the alternative these days, if we need to endorse it to end-users?

Related: CuyZ/Valinor#6
Related: https://github.com/phpstan/phpstan-src/tree/447cd102d946a2d9883ac82793195275f54296a9/src/Type
Related: https://github.com/vimeo/psalm/tree/f1d47cc662500589e200a978e3af85488a5236cf/src/Psalm/Type
Related: https://github.com/phpstan/phpdoc-parser
Related: https://github.com/phpstan/phpdoc-parser/blob/master/tests/PHPStan/Parser/TypeParserTest.php#L64

Overall, I'd like to at least explain (in the docs) that this library "ends" at providing docblocks, and that parsing them should be deferred to third-party libraries (since types are ever evolving there)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phpdoc-parser is really low-level, good comparison is nikic/PHP-Parser. In order to do something useful, you'd need parts of PHPStan that are hard to separate, like reflection layer, and type system.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I acknowledge that phpdoc-parser can sometimes be useful on its own, for example Slevomat CS uses it :) But it needs to interpret the AST on its own.

src/Reflection/ReflectionParameter.php Show resolved Hide resolved
Ocramius added a commit to Roave/BackwardCompatibilityCheck that referenced this pull request Dec 5, 2021
…Reflection@1f7a13b

This also handles the upcoming BC break of Roave/BetterReflection#900

Some big changes:

 * `PropertyDocumentedTypeChanged` no longer exists: we use only `PropertyTypeChanged` for now. This
   is a step backwards, but it is needed to prevent issues around docblocks being too advanced for
   this library to parse them, for now.
    * see CuyZ/Valinor#6
    * see https://github.com/phpstan/phpstan-src/tree/447cd102d946a2d9883ac82793195275f54296a9/src/Type
    * see https://github.com/vimeo/psalm/tree/f1d47cc662500589e200a978e3af85488a5236cf/src/Psalm/Type
    * see https://github.com/phpDocumentor/ReflectionDocBlock/blob/622548b623e81ca6d78b721c5e029f4ce664f170/src/DocBlock/Tags/Param.php#L35
    * see https://github.com/phpstan/phpdoc-parser/blob/d639142cf83e91a07b4862217a6f8aa65ee10d08/tests/PHPStan/Parser/TypeParserTest.php#L64
    * /cc @tomasnorre this sadly effectively reverts #340 - it
      is too much effort to dive into further advanced types :-(
 * `PropertyTypeChanged` only expects the property to be both covariant and contravariant: if not,
   then a BC break was introduced (this re-uses some internal variance checks, simplifying code
   substantially)
 * usage of `ReflectionFunctionAbstract` is no longer present in the library: always using an union
   of `ReflectionMethod|ReflectionFunction` instead (as in `roave/better-reflection:^5`)
 * `FunctionName` is now `@internal`, since it's only used for internal rendering purposes
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kukulich!

@Ocramius Ocramius self-assigned this Dec 5, 2021
@Ocramius Ocramius added BC break dependencies Pull requests that update a dependency file enhancement labels Dec 5, 2021
@Ocramius Ocramius added this to the 5.0.0 milestone Dec 5, 2021
@Ocramius Ocramius merged commit c6c9b85 into Roave:5.0.x Dec 5, 2021
@Ocramius
Copy link
Member

Ocramius commented Dec 5, 2021

Thanks @kukulich!

@kukulich kukulich deleted the types-finder branch December 5, 2021 09:31
uzibhalepu added a commit to uzibhalepu/BackwardCompatibilityCheck that referenced this pull request Aug 6, 2024
…Reflection@1f7a13b

This also handles the upcoming BC break of Roave/BetterReflection#900

Some big changes:

 * `PropertyDocumentedTypeChanged` no longer exists: we use only `PropertyTypeChanged` for now. This
   is a step backwards, but it is needed to prevent issues around docblocks being too advanced for
   this library to parse them, for now.
    * see CuyZ/Valinor#6
    * see https://github.com/phpstan/phpstan-src/tree/447cd102d946a2d9883ac82793195275f54296a9/src/Type
    * see https://github.com/vimeo/psalm/tree/f1d47cc662500589e200a978e3af85488a5236cf/src/Psalm/Type
    * see https://github.com/phpDocumentor/ReflectionDocBlock/blob/622548b623e81ca6d78b721c5e029f4ce664f170/src/DocBlock/Tags/Param.php#L35
    * see https://github.com/phpstan/phpdoc-parser/blob/d639142cf83e91a07b4862217a6f8aa65ee10d08/tests/PHPStan/Parser/TypeParserTest.php#L64
    * /cc @tomasnorre this sadly effectively reverts Roave/BackwardCompatibilityCheck#340 - it
      is too much effort to dive into further advanced types :-(
 * `PropertyTypeChanged` only expects the property to be both covariant and contravariant: if not,
   then a BC break was introduced (this re-uses some internal variance checks, simplifying code
   substantially)
 * usage of `ReflectionFunctionAbstract` is no longer present in the library: always using an union
   of `ReflectionMethod|ReflectionFunction` instead (as in `roave/better-reflection:^5`)
 * `FunctionName` is now `@internal`, since it's only used for internal rendering purposes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC break dependencies Pull requests that update a dependency file enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: Remove TypesFinder
3 participants