[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 #8278 (comment). 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. , #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.

[greg0ire]

I'll defer to @stof regarding your question about whether this is going in the right direction.

[greg0ire]

The docs need to be updated at the very least because of this. Can you also read it and see if anything else needs to be modified, and whether any new behavior you are adding is worth documenting?

[greg0ire]

Why is this necessary?

[d-ph]

You mean the catching of the MappingException?

It's primarily needed because this is how ClassMetadata::getSingleIdentifierFieldName() communicates that "the identifier is not single column". One way to avoid the try..catch is to duplicate the if-conditions from ClassMetadata::getSingleIdentifierFieldName() in here, but having a code duplication here would introduce a maintenance cost, which I personally consider a bigger nuisance than having a try..catch block.

Also, EntityManager::getClassMetadata() throws that exception class when the $className is unrecognised. Instead of making an assumption that "there must be metadata for every class detected at this point of the code", I wanted to be more graceful and defensive, and chose to just catch the exception and assume that the "auto-detection" cannot be carried out.

[greg0ire]

We usually avoid using exception for flow control, and I think that's because they are costly.

I think I would add a dedicated method, so as to remove the need of casting the result to a boolean as well, even if it means a little duplication.

Also, EntityManager::getClassMetadata() throws that exception class when the $className is unrecognised. Instead of making an assumption that "there must be metadata for every class detected at this point of the code", I wanted to be more graceful and defensive, and chose to just catch the exception and assume that the "auto-detection" cannot be carried out.

In what case will $className not be recognized and we wouldn't want to let the user know about that?

[d-ph]

Although I agree that resorting to using exceptions has downsides, I personally consider those downsides less painful than the alternative solutions, one of which in this case being an introduction of a maintenance burden by introducing code duplication (or an awkward "marshalling" of error reasons).

Note that even the mentioned article says that mere usage of try..catch (without actually expecting for the "catch path" to be routinely triggered) is negligible.

I'm not going to argue here and will introduce the code duplication (i.e. I will create a ClassMetadata::hasSingleIdentifierFieldName()).

In what case will $className not be recognized and we wouldn't want to let the user know about that?

I can't think of any "normal" case when it would be unrecognised here. So in this regard, I agree that it's better to let the user know that something strange happened.

[greg0ire]

The UPGRADE.md needs to document the deprecations.

[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]

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.

[stof]

@d-ph do you intent to resume that work ?
This PR got closed due to the deletion of the 3.3.x branch.

[greg0ire]

Oups! Sorry for this!

[d-ph]

Hi, yes, it's still in my plans. Thanks for clarifying why the PR got closed.

Just to manage expectations (if there are any): as everyone, I struggle for time. I do hope to progress this PR within 3 months.

Also, my last comment in this PR was a request to re-review it, @stof. I don't immediately remember what there still is to review, however it'd be worthwhile to hear whether you are generally ok with the scope of change in this PR. If I recall correctly, my plan a year ago was to get your "approval" (at least in principle), and then to finish the docs and cleanup, and finally request one last review.

Thanks.