emteknetnz
commented
1 year ago With both options I've noticed that you're passing an array instead of the documented methods of passing in strings ->eagerLoad('Relation01', 'Relation02.SubRelation')
Probably I think we could make this a little cleaner if we add a new method for this new functionality e.g.
public function eagerLoadRelation(string $relation, ?callable $callback = null): static { ... }
// ...
$myDataList->eagerLoadRelation('SomeRelation');
$myDataList->eagerLoadRelation('Members.FavouriteProducts', function (EagerLoadedDataList $products) {
return $products->exclude('Price:GreaterThan', 100);
});
GuySartorelli
maxime-rainville
Description
@kinglozzer pointed out in https://github.com/silverstripe/silverstripe-framework/issues/10874#issuecomment-1641674618 that it could be useful (and perhaps more performant) to pre-filter and pre-sort eager loaded data ahead of time, so that the filtering and sorting of that data doesn't have to be done in PHP. This is something I've heard echoed by a few people now when talking about the new eager loading functionality.
There are two ways I can see to implement this outlined below. With both approaches, the relation to be eager loaded becomes the key to the array. This is backwards compatible, because we can add a
is_numeric()check on the array keys, and if they are numeric we have no pre-filtering and just use the current implementation, and if they are not numeric, then we have pre-filtering and can do whatever needs to be done to pre-filter the results. In other words, if there is no filtering to be done, then relation chains should just be passed in as values, like they already are.In either scenario, we should throw exceptions if the relation to be eager loaded is either
has_oneorbelongs_to, since there is no list to be filtered or excluded in that scenario.Option one: Pass the unfiltered fetched records into a callback for filtering
This option might require heavier unit testing, but requires less code to implement and is more powerful as it gives the full
DataListAPI for filtering, sorting, or whatever else the developer wants to do (e.g. altering the underlying query, etc).It is similar to the approach originally proposed by KingLozzer, but sets the relation to eagerload as the key in the array with the callback as the value which allows it to be backwards compatible as discussed above.
and in
DataListwe would just pass the list of fetched rows to the callback like this:Option two: Define filters/sorts/etc in an array, and apply them as appropriate
This option is very simple, but any functionality has to be given an explicit support - i.e. if we don't explicitly check for and apply functionality for "where" as an array key in the filters array, developers won't be able to use SQL where queries to pre-filter their results. Because we have to explicitly support functionality, it makes it easier to test - but way less flexible.
They developer would use the API like this:
and in
DataListwe would just check for each supported thing in the filter array. e.g.Recommendation
Personally I think it's better to provide more flexibility for something like this, especially given eager loading is intended as an opt-in performance optimisation, so developers should have the freedom to squeeze every bit of performance they can out of it. In other words, I recommend option one, where developers provide a callback for filtering.
Acceptance Criteria
Note
PRs