Skip to content

Add current form data as second argument to RecordFinder scope method - #1441

Open
mjauvin wants to merge 2 commits into
developfrom
record-finder-scope
Open

mjauvin wants to merge 2 commits into
developfrom
record-finder-scope

Conversation

@mjauvin

@mjauvin mjauvin commented Jan 18, 2026 •

Copy link
Copy Markdown
Member

It can be useful if the record finder needs some other fields in the form to filter its values.

Related: wintercms/docs#256

E.g. fields definition:

country:
    type: dropdown

state:
    type: recordfinder
    list: $/author/plugin/models/state/columns.yaml
    scope: statesByCountry
class State extends model
{
    public function scopeStatesByCountry($query, $model, $formData)
    {
        if ($country_id = array_get($formData, 'country')) {
            $query->where('country_id', $country_id);
        }
    }
}

Summary by CodeRabbit

  • New Features
    • Custom record lists can now use the parent form’s current save data when applying scopes, allowing results to reflect values entered in the form. If no parent form data is available, the scope receives an empty set of data.

@mjauvin
mjauvin requested a review from LukeTowers January 18, 2026 17:24
@mjauvin mjauvin self-assigned this Jan 18, 2026
@mjauvin mjauvin added the enhancement PRs that implement a new feature or substantial change label Jan 18, 2026
@AIC-BV

AIC-BV commented Jan 19, 2026 •

Copy link
Copy Markdown
Contributor

Works perfectly! Thanks!!!

public function scopeWhereProduct($query, $model, $formData)
{
    if ($productId = array_get($formData, 'product')) {
        return $query->where('product_id', $productId);
    }
    return $query->where('product_id', null); // returns an empty list in my case, so they will be forced to select a product first ;)
}

@AIC-BV

AIC-BV commented Mar 10, 2026

Copy link
Copy Markdown
Contributor

@mjauvin can this be merged?
I had trouble because I ran composer update

@mjauvin

mjauvin commented Mar 10, 2026

Copy link
Copy Markdown
Member Author

@AIC-BV did you test this PR as well?

@AIC-BV

AIC-BV commented Mar 10, 2026 •

Copy link
Copy Markdown
Contributor

@AIC-BV did you test this PR as well?

I copied the change to my local file and deployed it in production since Jan 18
Works perfectly

@mjauvin

mjauvin commented Mar 10, 2026

Copy link
Copy Markdown
Member Author

@LukeTowers any objection in merging this? There's a PR to the docs also in the description.

@mjauvin mjauvin added this to the 1.2.13 milestone Mar 13, 2026
@LukeTowers LukeTowers modified the milestones: 1.2.13, 1.3.0 Jun 10, 2026
@JonasPardon

Copy link
Copy Markdown

@LukeTowers Also running into this, could this be merged please? 🙏🏻

@LukeTowers

Copy link
Copy Markdown
Member

@mjauvin does this match any other scope method calls in the core? Seems somewhat arbitrary for your specific use case. @JonasPardon @AIC-BV can you provide examples of how you're using it?

@AIC-BV

AIC-BV commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

@LukeTowers concrete case from our webshop plugin (running patched in production since January):

An OrderProduct has a product recordfinder and a variant recordfinder. A variant only exists within a product, so the variant list has to be filtered by the product that was just picked — in the same form, which isn't saved yet. $this->model alone can't do it: on a create form product_id is still null, so you either list every variant in the catalogue or none.

product:
    type: recordfinder
    list: ~/plugins/aic/webshop/models/product/columns.yaml

variant:
    type: recordfinder
    list: ~/plugins/aic/webshop/models/variant/columns.yaml
    scope: whereProduct
public function scopeWhereProduct($query, $model, $formData = [])
{
    $productId = array_get($formData, 'product') ?: $model->product_id;

    return $query->where('product_id', $productId); // null => empty list, forces picking a product first
}

Same shape as Marc's country → state: any parent → child recordfinder pair hits this. And since PHP ignores extra arguments on userland methods, existing single-argument scopes keep working unchanged.

In short, picking a product fills in the variant picker
image

@zimudec

zimudec commented Oct 10, 2026 •

Copy link
Copy Markdown

@LukeTowers answering your Aug 26 question — yes: "call the model scope with one extra contextual argument" already has multiple precedents in the core, and RecordFinder was the odd one out. Forward-port status first for completeness: as authored this targets develop (Laravel 9); I verified the change merges cleanly onto the current wip/1.3 tip (9a2fe625, the 1.3.0 milestone line) and ran the checks below there — results apply to both lines.

The audit (your first question). Greping every dynamic method call on query builders in modules/backend + modules/system, there are 13 call sites; the changed line in this PR is the only one that changed. Of the 12 pre-existing lines, 10 are scope calls that receive contextual extra arguments (the precedents below) and the remaining 2 are the search plumbing around searchScope inside Lists.php ($query->$searchMethod(…) at :1991/:1997 — the where()/orWhere() wrappers that feed the search term + columns):

  • RelationController.php:856 (view[scope]) and :1026 (manage[scope]) — same list.extendQueryBefore setup as RecordFinder, and the scope receives the parent record ($query->$scopeMethod($this->model)) — i.e. a widget-external model object passed as context, exactly the kind of extra data this PR's $formData is.
  • Relation.php:145 — scope receives the parent model (whose unsaved in-form state is exactly the case at hand).
  • Filter.php:794–931 (6 sites: values, ranges ($after, $before), ($min, $max)) — scopes receive the filter selection as an extra argument; formData is the same idea one level up (what the containing form holds rather than what the filter panel holds).
  • Lists.php:1991–1997 — the search plumbing: $q->$scopeMethod($term, $columns) at :1992 feeds any search.scope (RecordFinder's own searchScope funnels through here, so RecordFinder scopes were already context-arg consumers for search), wrapped by the where()/orWhere() $searchMethod calls at :1991/:1997.

So the PR makes RecordFinder's scope consistent with the view[scope]/manage[scope] family in RelationController instead of being arbitrary: the scope for a record list of a form widget gets the containing form's current state, which is exactly what a parent→child finder needs.

The repro (your second question, "how you're using it"). @AIC-BV's Jan-posted OrderProduct → variant case is the canonical pattern; I reproduced it programmatically on the merged wip/1.3 tree (runtime harness: a form with two fields named product and variant plus two recordfinders — one scope declared with the PR's 3-arg signature and one with the pre-PR 2-arg signature — capturing the arguments they receive; the lists' list.extendQueryBefore event is triggered through prepareQuery(), the same code path the popup's AJAX uses). Literal result for the 3-arg scope:

  • the scope receives the unsaved values of the containing form as the third argument: formData = {"product":"fake-A-99","variant":"fake-B-88"} — with the containing form's model instance as $this->model (first extra arg, as today) and the DB untouched;
  • with the pre-PR line ($query->$scopeMethod($this->model)) simulated in the same process, a scope with the PR's 3-arg signature dies with ArgumentCountError: Too few arguments … 2 passed … exactly 3 expected — i.e. without this change the parent→child filter cannot exist, which matches AIC-BV having to patch the file instead of only shipping the scope.

Backwards compatibility. Also verified in the same run: a scope with the old 2-arg signature ($query, $model) receives all three arguments (argc == 3, arg 2 is the model) and continues to work — userland methods ignore extra arguments (documented behavior; the additional value is still retrievable via func_get_args() for anyone who wants it). modules/backend test suite on the merged tree: 301 tests, 718 assertions, 0 failures.

One docs note for PR #256: the examples use array_get($formData, 'product') — that's fine, Storm still ships and uses that helper (helpers-array.php), and it reads naturally in both branches; worth making sure the snippet shown there notes the $formData argument so copy-pasters know the scope signature is ($query, $model, $formData).

Comment thread modules/backend/formwidgets/RecordFinder.php Outdated
Co-authored-by: Héctor pavez <hectzimudec@gmail.com>
@coderabbitai

coderabbitai Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: df7a7ea3-5eaf-4fe4-93d0-e2f62062709d

📥 Commits

Reviewing files that changed from the base of the PR and between d4bd85f and a2de06c.


📒 Files selected for processing (1)
  • modules/backend/formwidgets/RecordFinder.php

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.



Walkthrough

When RecordFinder uses a configured scope method, it now passes the parent form’s save data as a second argument. If the parent form or save data is unavailable, it passes an empty array.

Priority: ➖ Normal

Estimated code review effort: 1 (Trivial) | ~3 minutes

Merge Risk: ⚪ Minimal · up to a2de0

RecordFinder scopes can now filter using current form data, including unsaved values. The fallback and existing scope-call compatibility are supported by the inspected implementation, with no concrete issue preventing merge.

Pre-merge checks | Passed 4 | Failed 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: passing current form data as a second argument to the RecordFinder scope method.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR


🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement PRs that implement a new feature or substantial change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants