Skip to content

Fix SaveAwareArgResolver ignored in nested mutations - #2784

Open
spawnia wants to merge 1 commit into
masterfrom
fix-nested-pre-save-arg-resolver
Open

Fix SaveAwareArgResolver ignored in nested mutations#2784
spawnia wants to merge 1 commit into
masterfrom
fix-nested-pre-save-arg-resolver

Conversation

@spawnia

@spawnia spawnia commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Fixes a regression from #2777 (v6.69.0) where SaveAwareArgResolver::runBeforeSave() was only honoured at the top level of a mutation. One level down inside nested HasMany/HasOne/ManyToMany/BelongsTo mutations, the pre-save resolver ran AFTER the child model was already persisted — silently discarding its attribute changes.

The root cause: ResolveNested defaulted to a partitioner that unconditionally routed all resolver-bearing args into the post-save set. Only ModelMutationDirective::executeMutation() overrode this with the pre-save-aware variant; all 12 nested resolver call sites used the broken default.

The fix collapses nestedArgResolversWithoutPreSave into nestedArgResolvers so the correct behavior is the default everywhere. When root is not a Model, behavior is unchanged. When root is a Model, SaveAwareArgResolver instances with runBeforeSave=true stay in the regular set so SaveModel executes them before persisting. The now-redundant explicit partitioner in ModelMutationDirective is removed.

The default arg partitioner in ResolveNested unconditionally routed all
arguments with resolvers into the post-save set. This meant
SaveAwareArgResolver::runBeforeSave() was only honoured at the top level
(where ModelMutationDirective explicitly passed the pre-save-aware
partitioner) but silently ignored one level down inside nested
HasMany/HasOne/ManyToMany/BelongsTo mutations.

Collapse the two partitioner methods into one that always respects
pre-save semantics. When root is not a Model, behavior is unchanged
(all resolvers go to nested). When root is a Model,
SaveAwareArgResolvers with runBeforeSave=true stay in the regular set
so SaveModel can execute them before persisting.
@spawnia
spawnia requested a review from Copilot July 28, 2026 14:50
@spawnia
spawnia marked this pull request as ready for review July 28, 2026 14:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes a regression where SaveAwareArgResolver::runBeforeSave() was not honored inside nested model mutations, causing pre-save arg resolvers to run after child models were already persisted.

Changes:

  • Make ArgPartitioner::nestedArgResolvers() pre-save aware by default (and remove the redundant nestedArgResolversWithoutPreSave() variant).
  • Remove the explicit partitioner override from ModelMutationDirective, relying on the corrected default behavior.
  • Add/extend integration test coverage for pre-save custom directives inside nested relations and add latitude/longitude test fields/columns.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/Execution/Arguments/ArgPartitioner.php Makes the default nested/regular argument partitioning honor SaveAwareArgResolver::runBeforeSave() for model roots and lifts pre-save resolvers out of @nest recursively.
src/Schema/Directives/ModelMutationDirective.php Removes the now-unnecessary custom arg partitioner override when constructing ResolveNested.
tests/Unit/Execution/Arguments/ArgPartitionerTest.php Updates unit tests to reflect the new default partitioning behavior and removes coverage for the deleted method.
tests/Integration/Schema/Directives/CreateDirectiveTest.php Adds integration coverage demonstrating pre-save custom directive behavior in nested HasMany and deeper nested relations.
tests/database/migrations/2018_02_28_000003_create_testbench_tasks_table.php Adds latitude/longitude columns to support new integration assertions.
tests/database/migrations/2018_02_28_000004_create_testbench_posts_table.php Adds latitude/longitude columns to support deep relation integration assertions.
tests/Utils/Models/Task.php Documents latitude/longitude properties for test model.
tests/Utils/Models/Post.php Documents latitude/longitude properties for test model.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants