Entity queries with sorting and ordering are not working in a non-default workspace

The bug here is caused by our entity query alterations, not by the workspace switching functionality :)

Here's an attempt to fix this, let's see what the tests have to say.

My approach is to take the fields we are adding to the query (id and revision fields) and add them to the GROUP BY clause too, if the query has something in the GROUP BY clause already. At the moment this only happens when sorting complex queries, but I didn't wat to use sorting as a condition, as in the future we may find there are other ways for the query to have grouping.

Another atemp, it seems on some queries the GROUP BY is empty even if the aggregation functions MIN/MAX are used for sorting. For these cases we need another condition to detect if we should add the fields to the GROUP BY clause. We check if the query has sorting and it's not a simple query, specifically the case where the error happens.

I missed the fact that the sorting processing happens after we prepare the query, so I moved the checks to the addSort() method, after the parent method is executed. At this point we should know if we need to add or not the id fields to the GROUP BY clause.
Let's see what the tests have to say.

The tests passed :)

When I applied above patch, the issue with my entity query was indeed resolved.
The fatal error disappeared.

However, I noticed that there were a couple of issues with my entity list overviews.
Several entities were missing.

I found the issue and I'll try to explain:

Say I have 3 entities with following ids, reference ids:
|id|reference_id|
|1|1|
|2|2|
|3|5|

When I do not add sorting, the result I get is a keyed array:

$ids = [
 '1' => '1',
 '2' => '2',
 '5' => '3',
];

Entities can then be used by doing a loadMultiple($ids);
When the EntityQuery runs it's result function, the following piece of code is called:

return $this->sqlQuery->execute()->fetchAllKeyed();

The FetchAllKeyed method will use index 0 as it's key index, and index 1 as it's value index.

When the SQL query is altered by workspaces, 2 expressions are added in our Workspaces Query (Drupal\workspaces\EntityQuery\Query):

$this->sqlExpressions[$revision_field] = "COALESCE(workspace_association.target_entity_revision_id, base_table.$revision_field)";
$this->sqlExpressions[$id_field] = "base_table.$id_field";

And these are attached in the "finish" function:

protected function finish() {
    foreach ($this->sqlExpressions as $alias => $expression) {
      $this->sqlQuery->addExpression($expression, $alias);
    }
    return parent::finish();
}

When you don't have a sort, the result key will be the "revision" expression, and the result value will be the "id" expression.

However, when a query has a sort, this will also add an expression in the "addSort" method. And this expression will be added before the above 2 expressions. Because the expression is added in the "addSort" method instead of the "finish" method.

When you for example sort on "id" => "ASC", the following expression will be added:

min({entity}.id) AS expression

Your raw result set will suddenly be:
|1|1|1|
|2|2|2|
|3|5|3|

Your keyed results will then be:

$ids = [
 '1' => '1',
 '2' => '2',
 '3' => '5',
];

Meaning the last entity will not be found as it looks for an entity with an id of 5, which will not exist.

I have fixed this in the attached patch by adding the expressions immediately in the prepare function. It also contains the patch functionality in #12 by mariancalinro, as this is needed to avoid the fatal error.

This patch needs review and was tested on 8.6.5.

In the latest patch, it seems the test coverage from #12 is missing.

Thanks @mariancalinro and @mheip, the patches and the explanation from #14 are excellent detective work on the internals of entity query :)

I've just proposed a patch that changes the way entity query generates its SQL query in #2950869-30: Entity queries querying the latest revision very slow with lots of revisions. That patch also fixes this problem in Workspaces, so I've credited both of you in that issue for the helpful comments and solution here. If that patch gets in, the only goal of this issue would be to add the additional test coverage.

Since #2950869: Entity queries querying the latest revision very slow with lots of revisions doesn't have a chance of getting into 8.7.0 anymore, let's get back to fixing this in Workspaces for now.

Reading all the comments and patches again, I think we should combine the approaches from #10 and #14. The problem with sorting described in #14 can be solved by providing an override for the isSimpleQuery() method, and can be tested by asserting the result array of the entity query.

At the moment, this bug prevents Workspaces from loading the node edit page, so I'm raising the priority to major.

The patch looks good and is quite simple! The additional tests adds great coverage too!

Manual testing is trivial - the node form was broken on non-live workspaces (like stage), and it's no longer broken :)

Manually tested this as well, I wasn't able to get a broken form but the STR mentioned in the OP worked for me. I was a bit concerned about always adding the group by clause with complex queries but then I realized that the parent class does the same, so this change shouldn't have a performance impact.

Great detective work, guys!

Committed 073cc74 and pushed to 8.8.x and backported to 8.7.x. Thanks!

I had to revert the initial commit as I erroneously committed/pushed to 8.7.x first instead of cherry-picking from 8.8.x. The following two commits fix this and the latter has a nice reference to the 8.8.x commit, as it should. Sorry for the confusion :)