Split TreeWalker and SqlWalker

[derrabus]

When working on native types for the TreeWalker interface and its implementations I ran into some trouble. The main problem was that the interface declares most of its methods with a string return type while some implementations change the return type to void. This of course is not allowed and PHPStan and Psalm already complain a lot about that.

Analyzing the implementations and their usages, I came to the conclusion that we basically have two types of implementations:

• tree walkers that traverse the AST and modify it
• output walkers that emit SQL code while traversing the AST

Tree walkers are never called directly. Instead, they are loaded into a TreeWakerChain that iterates over them. Those walkers are optional; it might very well be that the chain is empty when parsing a query. On the other hand, we'll always have an output walker in that process. It has to be either a subclass of SqlWalker or SqlWalker itself which is the default.

You can verify my assumption by removing the implements part from SqlWalker: Doing so won't break a single test.

orm/lib/Doctrine/ORM/Query/SqlWalker.php

Line 48 in 3849aed

class SqlWalker implements TreeWalker

The TreeWalker interface is really quite large. This is why implementations (except for SqlWalker and TreeWalkerChain) usually extend TreeWalkerAdapter which provides empty implementations (with void return type!) for all methods. The custom tree walker would just override the method it really wants to implement. The interesting part about that is that only walkSelectStatement(), walkUpdateStatement() and walkDeleteStatement() are ever called publicly. Overriding any other method would be futile. Those methods are called on the output walker however.

This is why I'd like to propose the following changes:

• SqlWalker does not extend TreeWalker anymore.
• TreeWalker, TreeWalkerAdapter and TreeWalkerChain are reduced to walkSelectStatement(), walkUpdateStatement() and walkDeleteStatement() and receive a consistent void return type.

[morozov]

The main problem was that the interface declares most of its methods with a string return type while some implementations change the return type to void. This of course is not allowed and PHPStan and Psalm already complain a lot about that.

Without looking too much into the implementation, instead of having multiple interfaces, would it be possible to have one parameterized? See https://phpstan.org/r/7ec5df39-c547-4116-abd8-f706ae0dd548 for example.

[derrabus]

Without looking too much into the implementation, instead of having multiple interfaces, would it be possible to have one parameterized?

That's possible and that way we could have SqlWalker implement TreeWalker still, but it would also mean that we would never be able to add a native return type to the interface. I doubt that this is the right solution. Tree walkers are never used in places where we use output walkers and vice versa. They don't need to share a common abstraction.

[derrabus]

@greg0ire @beberlei This PR has not really gotten much attention, but I'd like to work on this for 2.13 because it removes a major point of confusion for both static analysis tools. Feedback would be very much appreciated.

[greg0ire]

I think your proposal goes in the right direction! It's not like we have a big bag of walkers at some point and iterate on them, they seem to be well separated from the start.

I used this as an occasion to try out the MermaidJS integration (disregard the SomeOtherInterface, I could not find a way to make the lone SqlWalker appear otherwise):

Before

classDiagram
TreeWalker <|-- TreeWalkerAdapter
TreeWalker <|-- TreeWalkerChain
TreeWalkerChain *-- TreeWalker
TreeWalker <|-- SqlWalker
TreeWalker : string walkSelectStatement()
TreeWalker : string walkUpdateStatement()
TreeWalker : string walkDeleteStatement()
TreeWalker : mixed manyMoreMethods()

Loading

After

classDiagram
TreeWalker <|-- TreeWalkerAdapter
TreeWalker <|-- TreeWalkerChain
TreeWalkerChain *-- TreeWalker
TreeWalker : void walkSelectStatement()
TreeWalker : void walkUpdateStatement()
TreeWalker : void walkDeleteStatement()
SomeOtherInterface <|-- SqlWalker

Loading

Regarding the purpose of manyMoreMethods(), they do seem useless in TreeWalkerChain and TreeWalkerAdapter, but I will let @beberlei confirm that.

[derrabus]

Rebased. Now that the deprecation is in place, I think I can backport some more changes to 2.13. Leaving this PR as draft for now.

[derrabus]

I'm not really happy with those unions here. We should try to get rid of them in a follow-up.