[PR #9551] Split TreeWalker and SqlWalker #11700

New Issue

admin · 2026-01-22T16:11:38+01:00

admin commented

2026-01-22 16:11:38 +01:00

Original Pull Request: https://github.com/doctrine/orm/pull/9551

State: closed
Merged: Yes

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.

3849aed6fb/lib/Doctrine/ORM/Query/SqlWalker.php (L48)

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.

**Original Pull Request:** https://github.com/doctrine/orm/pull/9551 **State:** closed **Merged:** Yes --- 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. https://github.com/doctrine/orm/blob/3849aed6fb47047d2b32b2109b8486e15447cd5f/lib/Doctrine/ORM/Query/SqlWalker.php#L48 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.

admin added the pull-request label 2026-01-22 16:11:38 +01:00

Sign in to join this conversation.

Branches Tags

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: doctrine/archived-orm#11700