Eagerload some needed relations in ListDiscussionsController #2639
Conversation
| @@ -110,9 +110,13 @@ protected function data(ServerRequestInterface $request, Document $document) | |||
|
|
|||
| Discussion::setStateUser($actor); | |||
|
|
|||
| if (in_array('mostRelevantPost.user', $include)) { | |||
There was a problem hiding this comment.
I'd add a comment saying this is done for eager loading
There was a problem hiding this comment.
Do we want to explain why we need to eager load it or just state that we're eager loading it ?
Observation: this is currently only necessary because the suspend extension's UserPolicy calls isAdmin on the $user object which loads all groups, in which case we should be eager loading the relation in the extension and not here, but I'm seeing in https://github.com/flarum/core/pull/2620/files#diff-5f5aff0db193a1b7b5bdff796982aefe84572973059c31c65f33233e7fafd597R34-R36 that we might end up doing it in core as well, so it will belong here after all (if that pr isn't changed).
There was a problem hiding this comment.
Should the comment reflect that? Suggestion:
// Eager load groups for use in the policies (isAdmin check)
SychO9
force-pushed
the
sm/2637-list-discussions-eagerloading
branch
from
36f855b
to
b5c63ab
Compare
askvortsov1
force-pushed
the
sm/2637-list-discussions-eagerloading
branch
from
b5c63ab
to
f32b471
Compare
| @@ -110,9 +110,13 @@ protected function data(ServerRequestInterface $request, Document $document) | |||
|
|
|||
| Discussion::setStateUser($actor); | |||
|
|
|||
| if (in_array('mostRelevantPost.user', $include)) { | |||
There was a problem hiding this comment.
Should the comment reflect that? Suggestion:
// Eager load groups for use in the policies (isAdmin check)
SychO9
force-pushed
the
sm/2637-list-discussions-eagerloading
branch
from
22f6354
to
885f454
Compare
Part Of #2637
Changes proposed in this pull request:
Ensures some needed relations are present/eagerloded.
Can be tested by going to
/api/discussions?filter[q]=ipsumScreenshot
⬇️
Confirmed
composer test).