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.

Sign up for GitHub

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
Merged

Document tree walker class strings #9553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/Doctrine/ORM/Query/Parser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

*/
private $customOutputWalker;

Expand All @@ -245,6 +245,7 @@ public function __construct(Query $query)
* This tree walker will be run last over the AST, after any other walkers.
*
* @param string $className
* @psalm-param class-string<SqlWalker> $className
*
* @return void
*/
Expand All @@ -257,7 +258,7 @@ public function setCustomOutputTreeWalker($className)
* Adds a custom tree walker for modifying the AST.
*
* @param string $className
* @psalm-param class-string $className
* @psalm-param class-string<TreeWalker> $className
*
* @return void
*/
Expand Down
5 changes: 4 additions & 1 deletion lib/Doctrine/ORM/Query/SqlWalker.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
* the corresponding SQL.
*
* @psalm-import-type QueryComponent from Parser
* @psalm-consistent-constructor
*/
class SqlWalker implements TreeWalker
{
Expand Down Expand Up @@ -172,7 +173,9 @@ class SqlWalker implements TreeWalker
private $quoteStrategy;

/**
* {@inheritDoc}
* @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).
Comment on lines +176 to +178
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 •
edited
Loading

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.

*/
public function __construct($query, $parserResult, array $queryComponents)
{
Expand Down
5 changes: 0 additions & 5 deletions phpstan-baseline.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,11 +975,6 @@ parameters:
count: 1
path: lib/Doctrine/ORM/Query/SqlWalker.php

-
message: "#^Property Doctrine\\\\ORM\\\\Query\\\\SqlWalker\\:\\:\\$query \\(Doctrine\\\\ORM\\\\Query\\) does not accept Doctrine\\\\ORM\\\\AbstractQuery\\.$#"
count: 1
path: lib/Doctrine/ORM/Query/SqlWalker.php

-
message: "#^Result of && is always false\\.$#"
count: 2
Expand Down
16 changes: 4 additions & 12 deletions psalm-baseline.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2317,24 +2317,16 @@
<PossiblyUndefinedVariable occurrences="1">
<code>$args</code>
</PossiblyUndefinedVariable>
<PropertyNotSetInConstructor occurrences="1">
<code>$customOutputWalker</code>
</PropertyNotSetInConstructor>
<PropertyTypeCoercion occurrences="2">
<code>$className</code>
<code>$this-&gt;customTreeWalkers</code>
</PropertyTypeCoercion>
<RedundantConditionGivenDocblockType occurrences="4">
<code>$AST instanceof AST\SelectStatement</code>
<code>is_string($functionClass)</code>
<code>is_string($functionClass)</code>
<code>is_string($functionClass)</code>
</RedundantConditionGivenDocblockType>
<UnsafeInstantiation occurrences="4">
<UnsafeInstantiation occurrences="3">
<code>new $funcClass($funcNameLower)</code>
<code>new $funcClass($funcNameLower)</code>
<code>new $funcClass($funcNameLower)</code>
<code>new $outputWalkerClass($this-&gt;query, $this-&gt;parserResult, $this-&gt;queryComponents)</code>
</UnsafeInstantiation>
</file>
<file src="lib/Doctrine/ORM/Query/ParserResult.php">
Expand Down Expand Up @@ -2411,6 +2403,9 @@
<code>$this-&gt;queryComponents[$factor]['token']['value']</code>
<code>$this-&gt;queryComponents[$term]['token']['value']</code>
</InvalidScalarArgument>
<MoreSpecificImplementedParamType occurrences="1">
<code>$query</code>
</MoreSpecificImplementedParamType>
<PossiblyInvalidArgument occurrences="2">
<code>$aggExpression-&gt;pathExpression</code>
<code>$whereClause-&gt;conditionalExpression</code>
Expand Down Expand Up @@ -2443,9 +2438,6 @@
<PossiblyNullReference occurrences="1">
<code>dispatch</code>
</PossiblyNullReference>
<PropertyTypeCoercion occurrences="1">
<code>$query</code>
</PropertyTypeCoercion>
<RedundantConditionGivenDocblockType occurrences="3">
<code>$likeExpr-&gt;stringPattern instanceof AST\InputParameter</code>
<code>$whereClause !== null</code>
Expand Down