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

Fix nested Repeater Eager Loading #9802

Merged
merged 5 commits into from Nov 22, 2023
Merged

Fix nested Repeater Eager Loading #9802

merged 5 commits into from Nov 22, 2023

Conversation

johnwinkcq
Copy link
Contributor

@johnwinkcq johnwinkcq commented Nov 22, 2023

Checks if the relationship is already eagerly loaded, returning itself to avoid querying the database again and causing an N+1 query problem.

Issue/Bug Description: #9776

  • Changes have been thoroughly tested to not break existing functionality.
  • New functionality has been documented or existing documentation has been updated to reflect changes.
  • Visual changes are explained in the PR description using a screenshot/recording of before and after.

Checks if the relationship is already eagerly loaded, returning itself to avoid querying the database again and causing an N+1 query problem.
@johnwinkcq
Copy link
Contributor Author

Local composer test:
image

@danharrin danharrin added the bug Something isn't working label Nov 22, 2023
@danharrin danharrin added this to the v3 milestone Nov 22, 2023
@danharrin
Copy link
Member

Your previous implementation would have broken modifyRelationshipQueryUsing() and orderColumn(). I've added in checks to ensure that this is not used when either of those is set. If you want to use them with eager loading, you must add these clauses to the eager loading itself. Does it still work ok for your use case?

@danharrin danharrin linked an issue Nov 22, 2023 that may be closed by this pull request
@johnwinkcq
Copy link
Contributor Author

Your previous implementation would have broken modifyRelationshipQueryUsing() and orderColumn(). I've added in checks to ensure that this is not used when either of those is set. If you want to use them with eager loading, you must add these clauses to the eager loading itself. Does it still work ok for your use case?

would sortBy fix the orderColumn Problem?

elseif (
    $this->getModelInstance()->relationLoaded($relationshipName) &&
    (! $this->modifyRelationshipQueryUsing) &&
    filled($orderColumn)
){
    return $this->cachedExistingRecords = $this->getRecord()->{$relationshipName}->sortBy($orderColumn)->mapWithKeys(
        fn (Model $item): array => ["record-{$item[$relatedKeyName]}" => $item],
    );
}

That would take the eager loading into account & still work, wouldn't it?

@danharrin
Copy link
Member

It would fix it, but I don't know why you would want to do that, when you can chain it onto the with() anyway? It's more performant if done on the query instead of the collection

@johnwinkcq
Copy link
Contributor Author

johnwinkcq commented Nov 22, 2023

In my case it does not work.
If I use ->orderColumn() I can sort & save it.
If I use ->reorderable() I can sort it but when I save it, it resets itself. and also a reload of the page shows the status before sorting.

However, I have applied orderBy('sort') directly to the relationship

johnwinkcq and others added 2 commits November 22, 2023 10:33
Feature for orderColumn + Eager Loading
@danharrin danharrin merged commit 8bef2bb into filamentphp:3.x Nov 22, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Eager Loading Bug - 2...n nested Repeater
2 participants