Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow using callbacks with load relations extender #3116

Merged
merged 8 commits into from
Nov 1, 2021

Conversation

SychO9
Copy link
Member

@SychO9 SychO9 commented Oct 21, 2021

Needed for flarum/tags#149

Changes proposed in this pull request:
Some more complex relations can require eager loading with a callback, it is currently not possible, which is what this PR is for.

More explanation about the specific issue this helps solve in the related tags PR.

Reviewers should focus on:
This all needs to be backwards compatible of course, is it the case or am I missing something ?

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I like this, but I have a few concerns:

  • we're mixing arrays with integer-indexed and string-index elements. This feels very off to me, but I'm not sure I have a better idea: the alternative would be separate extender methods, but that feels a bit clunky and confusing.
  • Although the type system won't freak out, if the request is not given, the callback could fail with a null pointer exception.

More generally though, the load system is starting to feel pretty convoluted and complicated. In this case, it feels like it should be done through the tag model's withStateFor method. Maybe that should somehow be baked into the relation? But I also wonder if there's some way we can determine what we need to explicitly load based on include information without having to specify this directly at all.

@SychO9
Copy link
Member Author

SychO9 commented Oct 28, 2021

Been thinking about this. I have tweaked the current system to keep load as a string|string[] consumer, while adding a new loadWhere method for callbacks, the array itself within the controller was also split.

This is how it looks like in tags now, the reason we tap into the tags relation instead is so that we can call withStateFor query scope from the Tag model, because the query here is a tags relation query, that means the callback needs to also be aware of the array of relations that are to be loaded.

(new Extend\ApiController(ListDiscussionsController::class)
    ->loadWhere('tags', function ($query, $request, $relations) {
        if ($request && in_array('tags.state', $relations, true)) {
            $query->withStateFor(RequestUtil::getActor($request));
        }
    }),

But I also wonder if there's some way we can determine what we need to explicitly load based on include information without having to specify this directly at all.

I don't see that being possible, apart from needing to eager load what is to be included in the JSON response, other models to eager load depend on what relations we access to use in the code logic.

@SychO9 SychO9 merged commit 2b16302 into master Nov 1, 2021
@SychO9 SychO9 deleted the sm/tags-state-performance-fix branch November 1, 2021 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants