[Draft][Please review] Make Paginator use simpler queries when there are no sql joins used (#8278)

[d-ph]

May I ask for an initial review of the code in this PR before I move forward with it, please?

This PR implements the scope of work discussed in https://github.com/doctrine/orm/issues/8278#issuecomment-2239306885. It only implements the "no joins used" case. It doesn't implement the "only ToOne joins used" case. I plan do implement that other case in a future PR, after this one gets reviewed and hopefully merged.

A few comments:

1. This PR adds quite a few instance variables to the Paginator class, and also adds a few big docblocks that explain things. Please let me know whether this is acceptable.
2. As discussed in the github issue mentioned above, the feature introduced by this PR is only ever effective if the users explicitly use a certain argument value (namely: $fetchJoinCollection = null). This means that there could be a long period of time before the "auto-detection of better defaults" gets enabled by default (if ever).
3. The PR lacks unit tests. I plan to add them after I get an initial "ok" for this PR.
4. I see that this PR says that there are failing unit tests. They don't seem related to this PR's code change. I would look into reproducing them in my machine after the initial code review is done.

Thank you.

[greg0ire]

Triggering a new build to address 4. , https://github.com/doctrine/orm/pull/11596 should fix it.

[d-ph]

@greg0ire

Hi,

While I wait for some final answers from stof to my (regrettably) wall of text, may I know your initial thoughts on my proposition to rename some instance variables in the Paginator class, please?

I explained it in more details in the discussion above, but the bottom line is:

1. Rename Paginator::$fetchJoinCollection to something along the following line: $query(Potentially)ProducesDuplicateRows(ForTheRootAlias). It's just a change of a variable name, because the "semantics" of that variable would remain the same (i.e. it being a boolean, defaulted to "true"). The potentially undesired consequence is that the Paginator::getFetchJoinCollection() would have to remain in the class, and be deprecated (a new getter would also be added as a replacement).

2. Split Paginator::$useOutputWalkers into two variables: $useResultQueryOutputWalker and $useCountQueryOutputWalker. The potentially undesired consequence is that, again, the current getter and setter for that instance variable would be deprecated, and 4 new instance functions would be added: getters and setters for the 2 new variables. I'm not totally crazy about it myself, but it is what it is.

---

Would you say that you would initially allow for the changes described above to be made to the Paginator by me?

Thanks.

[greg0ire]

1. If fetchJoinCollection is truly inaccurate, then yes, I'd be open to renaming it, but maybe to something shorter like $queryProducesDuplicates. The ensuing deprecation is no big deal IMO.
2. Nothing to comment here, I think it's fine.

[d-ph]

Hi all,

I pushed another iteration of this PR, taking into account the feedback so far. Could you re-evaluate, please?

Major points:

1. This PR now goes farther than "agreed to". I.e. it analyses the query's AST to see what kind of JOINs are used, and what is being selected in the SELECT clause. If this is too far, then I may comment out some part of it, to "stagger" the release of this PR across multiple releases.
2. The routine that analyses the AST is about 150 lines long. If this is too cluttering, then the "auto detection" could perhaps be extracted to a separate "factory class". Or I could put that function to the bottom of that file, so that it's less distracting to the causal reader.
3. The "simple" count query is now used regardless of whether there are ToMany JOINs or not. I used the "list of conditions" from stof to determine whether the simple count query can be used, instead of the "complex" one. I personally consider this to be a bold move, so if it's too bold, then I can restore the original "proposition" of not allowing the "simple" count query to be used, when there are ToMany JOINs present.
4. Tests are still missing. I will put them in when I get a confirmation that the PR is going in the acceptable direction.

Thank you.

[d-ph]

@stof

Hi stof, would you be able to find some time in the coming weeks to see what you think about the new/current content of this PR, please?

[d-ph]

@stof

Hi stof. I was wondering whether taking a look at the current diff of this PR is still on your radar. I tried to action all the points that you raised (and where I couldn't, I hope that the middle ground is acceptable). Would you be able to share your thoughts on the current PR in the coming weeks, please?

[github-actions[bot]]

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. If you want to continue working on it, please leave a comment.

[d-ph]

I will continue this.