Add a nice way to create a builder for subqueries#7040
Add a nice way to create a builder for subqueries#7040derrabus merged 1 commit intodoctrine:4.4.xfrom
Conversation
|
cc @sbuerk |
There was a problem hiding this comment.
I had to implement a subquery functionality for my former employer when I migrated their codebase from a proprietary database layer to the DBAL. So definitely a +1 from me.
In that case, the parameters from the subquery would automatically get merged into the higher-level one (both positional and named). As far as I understand, in this case, the parameters of the subquery need to be set on the higher-level one which may be a little confusing. We've had this discussion about some other feature recently.
It would be nice to get this taken care of, but it's not a blocker.
UPD: here's the reference to what that implementation looked like: #2305 (comment).
| /** | ||
| * Returns a fresh query builder instance that can be used to build a subquery. | ||
| */ | ||
| public function sub(): self |
There was a problem hiding this comment.
Although it can be used to build a sub query, it does not have to, and the two instances maintain no link to each other. Did you consider other names for this method? From what I can tell , the important thing is that both QBs share the same connection. But maybe the intent expressed in the comment is more important than the implementation.
There was a problem hiding this comment.
I chose the name because creating a subquery is the usual use-case for calling that method. Yes, you can "abuse" it for other things, but why should I reflect this in the name?
That being said, we can change the name before the 4.4.x release. If you have a better idea, feel free to drop it here or open a PR. 🙂
There was a problem hiding this comment.
@derrabus Late to the party, personnaly I would prefer createQueryBuilder() to match the naming from the connection. Some reasonings:
- Emphasizes the method passthrough to the underlying
Connection. - It may be (mostly) used for subqueries, but nothing limits it (yet).
- Despite the reoccurance of the "merge parameter" topic, it may be a (hacky) way to connect the properties here so that the top container is used - which is okay for a "new instance" as no parameter needs to be merged at that state. But that would make the situation worser, and should be discussed (liked mentioned in the other comment) dedicatly.
Or createSubQueryBuilder() making it at least from the name clear ...
sub() would not be the method name coming for me in mind to get a new QueryBuilder instance.
|
Thanks for the reviews. I'm going to merge now because I believe we can continue all open discussions on other issues or follow-up PRs. Regarding merging parameters: Yes, on virtually every PR that deals with multiple QBs someone opens that discussion and it is always out of scope for those kind of PRs. We have an issue about that: #6525. Let's continue there. |
* 4.4.x: Add a nice way to create a builder for subqueries (doctrine#7040)
5 participants
Summary
In DBAL 3, we could ask q query builder for its database connection. That accessor has been removed in DBAL 4 which is a good thing.
Currently, I'm migrating a codebase that used the
QueryBuilder::getConnection()method in order to create a fresh builder for constructing a subquery.I believe this is a valid requirement, especially given that we allow query builders for unions and CTEs.
This PR proposes a new method
sub()which creates a fresh builder for the same connection, pretty much like the already existing methodexpr()does for the expression builder.