perf: reuse eager-loaded relations in forModel via loadMissing - #197
Merged
Conversation
BaseBuilder::forModel() unconditionally called
$model->load('customFieldValues.customField.options') for every
Infolist/Form schema generation. When the host application already
eager-loads those relations (e.g. a Filament resource's
getEloquentQuery() eager-loading custom field values so the table
avoids an N+1), load() refetched them anyway, so a view/edit page
ran the three relation queries twice.
Switching to loadMissing() reuses already-loaded relations and only
queries when they are absent, so the un-eager path is unchanged. This
matches the package's existing behaviour elsewhere
(BackendVisibilityService and LookupPreloader already guard on
relationLoaded()). The TableBuilder guard is untouched.
Note: loadMissing() no longer force-refetches, so if a record's custom
field values are mutated and the same in-memory instance is re-rendered
within one request, call $record->unsetRelation('customFieldValues')
first. Standard Filament view/edit flows re-resolve the record per
request and are unaffected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
BaseBuilder::forModel()unconditionally calls$model->load('customFieldValues.customField.options')on every Infolist/Form schema generation. When the host application already eager-loads those relations — e.g. a Filament resource'sgetEloquentQuery()loading custom field values so the table avoids an N+1 —load()refetches them anyway, so a view/edit page runs the three relation queries twice: once from the host's eager-load, once fromforModel().This switches
load()→loadMissing(), soforModel()reuses relations that are already loaded and only queries when they're absent.Why it's safe
loadMissing()still loads the relations when they aren't present (covered by a test).BackendVisibilityService::extractFieldValues()andLookupPreloaderalready guard onrelationLoaded()rather than force-loading.TableBuilderguard is untouched; only Infolist/Form/Exporter/Importer builders are affected, and Exporter/Importer callforModel(new $model)on a keyless instance where the load is inert.Caveat
loadMissing()no longer force-refetches. If code mutates a record's custom field values and re-renders the same in-memory instance within one request, it should$record->unsetRelation('customFieldValues')before re-rendering. Standard Filament view/edit flows re-resolve the record per request, so they're unaffected.Test
tests/Feature/Integration/Builders/ForModelLoadMissingTest.php:forModel()issues nocustom_field_valuesquery when the record is already eager-loaded (fails onload(), passes onloadMissing())Local run of the full CI gate passes: Pint, PHPStan, Rector, and Pest (785 passed).