fix: return memoized model field values as data instead of re-invoking them - #4252
Merged
Merged
Conversation
…g them Model::__get() memoizes resolved field values back into the fields array. On a later read of the same field on the same model instance, is_callable() matched memoized string values against defined PHP function names case-insensitively, so a value like "Max" invoked the builtin max() with no arguments and caused a fatal. Unresolved fields are always closures (wrap_fields() wraps every field), so the invoke path now requires an actual Closure and everything else is returned as data. This restores the type guard removed in v2.3.3. Regression tests cover the widest reproducing surface (a GraphQL query that reads the same field twice on one model instance via an alias) and the model layer directly.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4252 +/- ##
=========================================
- Coverage 84.2% 84.2% -0.0%
- Complexity 5625 5626 +1
=========================================
Files 291 291
Lines 23604 23607 +3
=========================================
+ Hits 19880 19882 +2
- Misses 3724 3725 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Both live in the same method as the memoization fix and were fixed once without a regression test, which is how they could silently break again: - Falsey memoized values (false/0/'') must survive a re-read. Guards the fix that changed the outer guard from ! empty() to isset()/array_key_exists() (verified: this test fails with "null is identical to false" if ! empty() is reintroduced). - Non-closure callback field definitions (callable arrays, the 'callback' shape) still resolve. Locks the wrap_fields() <-> __get() coupling the Closure-only guard depends on.
This branch was previously deployed
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
1 participant
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 bug does this fix? Explain your changes.
Model::__get()memoizes a field's resolved value back into the fields array. On a later read of the same field on the same model instance,is_callable()matched memoized string values against defined PHP function names case-insensitively, so a value like"Max"(a first name) invoked the builtinmax()with zero arguments and caused a fatal:Unresolved fields are always closures, because
wrap_fields()wraps every field definition in one, so the invoke path now requires an actualClosureand everything else is returned as data. This restores the type guard that was removed in the__get()simplification first shipped in v2.3.3.Any string value colliding with a function name ("Max", "Time", "Date", "List", "Sort", ...) could trigger this whenever the same field on the same model instance resolved more than once, for example a query requesting a field twice through an alias.
Does this close any currently open issues?
Fixes #4240
Testing Strategy
TDD: added
tests/wpunit/ModelFieldMemoizationTest.phpwith two tests, confirmed both fail against the unfixed code (the model-level test with theArgumentCountErrorfatal, the query-level test witherrorsin the response), then implemented the fix and confirmed both pass.viewer.firstNametwice via an alias, for a user named "Max Time" (both name parts collide with builtins)$model->firstNametwice directlyTest Results
Additional Context
Credit to @davidvexel for an excellent report, including pinpointing the regression to the
__get()refactor in #3381. The suggested fix in the issue would still invoke callable-shaped arrays; sincewrap_fields()guarantees unresolved fields are closures, this PR uses the tighterinstanceof \Closureguard so memoized arrays and strings are always treated as data.