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

Document tree walker class strings #9553

Merged
merged 1 commit into from
Mar 1, 2022

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Mar 1, 2022

Another backport from #9551

@derrabus derrabus added this to the 2.12.0 milestone Mar 1, 2022
Comment on lines +176 to +178
* @param Query $query The parsed Query.
* @param ParserResult $parserResult The result of the parsing process.
* @psalm-param array<string, QueryComponent> $queryComponents The query components (symbol table).
Copy link
Member Author

Choose a reason for hiding this comment

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

Psalm seems to ignore @inheritDoc on constructors. 🤷🏻‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

It is supposed to work though: vimeo/psalm#4504 (comment)

Copy link
Member

@greg0ire greg0ire Mar 1, 2022

Choose a reason for hiding this comment

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

But I think you'd need to add psalm-consistent-constructor on the interface as well (or in fact, only there? not sure if it's necessary to have it in this class). Can you please try that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't really work. If I add this annotation to the interface, @inheritdoc on SqlWalker's constructor is still ignored. If I remove the attotation from SqlWalker, @inheritdoc works again, but this also brings back the UnsafeInstantiation error for new $outputWalkerClass. 😕

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I guess defining a constructor in an interface is quite the corner case.

@@ -220,7 +220,7 @@ class Parser
/**
* The custom last tree walker, if any, that is responsible for producing the output.
*
* @var class-string<TreeWalker>
* @var class-string<SqlWalker>|null
Copy link
Member Author

Choose a reason for hiding this comment

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

As explained on #9551, output walkers cannot be arbitrary implementations of TreeWalker, we do expect an SqlWalker here.

@derrabus derrabus merged commit 1febeac into doctrine:2.12.x Mar 1, 2022
@derrabus derrabus deleted the types/tree-walker-class-strings branch March 1, 2022 19:14
n-e-m-a-nj-a pushed a commit to n-e-m-a-nj-a/orm that referenced this pull request Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants