FIX Use ORM functions to perform eager loading instead of raw query #11509
Conversation
| . ' ORDER BY FIELD(' . $childIDField . ', ' . $fetchedIDsAsString . ')' | ||
| ); | ||
| $joinRows = SQLSelect::create() | ||
| ->setSelect('"' . $joinTable . '".' . "*") |
There was a problem hiding this comment.
| ->setSelect('"' . $joinTable . '".' . "*") |
It will select everything by default, I believe.
There was a problem hiding this comment.
Removing the table-specific select may lead to ambiguity with the columns from the joined table. This ensures that we're always working with the correct values.
src/ORM/DataList.php
Outdated
| ); | ||
| $joinRows = SQLSelect::create() | ||
| ->setSelect('"' . $joinTable . '".' . "*") | ||
| ->setFrom([$joinTable => $joinTable]) |
There was a problem hiding this comment.
| ->setFrom([$joinTable => $joinTable]) | |
| ->setFrom('"' . $joinTable . '"') |
There's no need to set an alias or use an array, but we do need to escape the table name.
src/ORM/DataList.php
Outdated
| ->addWhere('"' . $parentIDField . '" IN (' . implode(',', $parentIDs) . ')') | ||
| ->addWhere('"' . $childIDField . '" IN (' . $fetchedIDsAsString . ')') |
There was a problem hiding this comment.
You need to include the table name in addWhere so there's no ambiguity if we change the query in the future.
| ->setFrom([$joinTable => $joinTable]) | ||
| ->addWhere('"' . $parentIDField . '" IN (' . implode(',', $parentIDs) . ')') | ||
| ->addWhere('"' . $childIDField . '" IN (' . $fetchedIDsAsString . ')') | ||
| ->addLeftJoin($childTable, "$childTable.ID = $joinTable.$childIDField") |
There was a problem hiding this comment.
Why do we need this join?
There was a problem hiding this comment.
Let's say we have a joinTable AB linking A's many_many to B. The original query preserved the order of the items that were fetched from table B by putting their IDs in the FIELD list. This changes that by joining table B so that the original order by used to fetch B items can also be used in this query.
The perfect solution would be to right join the join table to the relation query (line 1350), which removes the need for this query altogether, though I'm not sure what the implications will be. I'll have to look into that.
| ->addWhere('"' . $parentIDField . '" IN (' . implode(',', $parentIDs) . ')') | ||
| ->addWhere('"' . $childIDField . '" IN (' . $fetchedIDsAsString . ')') | ||
| ->addLeftJoin($childTable, "$childTable.ID = $joinTable.$childIDField") | ||
| ->setOrderBy($fetchedOrderBy) |
There was a problem hiding this comment.
What is this orderby? How do we know it's what we actually want?
There was a problem hiding this comment.
This order by is the replacement for the FIELD order by in the original query as explained in the other comment.
| $query = $fetchList->dataQuery()->query(); | ||
| $fetchedOrderBy = $query->getOrderBy(); | ||
| $childTables = $query->queriedTables(); | ||
| $childTable = reset($childTables); |
There was a problem hiding this comment.
Why do we arbitrarily take the first table?
There was a problem hiding this comment.
As far as I could tell this is the only way to derive the base table (the FROM) from a DataList query. An alternative is to grab the table name from the baseClass data object.
I'll add comments to my next commit to make clear what's happening exactly.
Description
The original query was a hardcoded and a rather hacky way of retaining the ordering of the child objects while fetching from a many_many table. The method used to retain this order was 'FIELD', which is a MySQL exclusive feature and breaks in other sql engines. This change converts the query to properly make use of the ORM functions and use the ordering used in the original query directly.
Manual testing steps
Issues
Pull request checklist