Skip to content

Trigger model events on nested mutation delete of belongsTo relationships#2588

Open
skaesdorf wants to merge 6 commits into
nuwave:masterfrom
SOERF:nested-belongs-to-delete-model-events
Open

Trigger model events on nested mutation delete of belongsTo relationships#2588
skaesdorf wants to merge 6 commits into
nuwave:masterfrom
SOERF:nested-belongs-to-delete-model-events

Conversation

@skaesdorf

@skaesdorf skaesdorf commented Jul 18, 2024

Copy link
Copy Markdown
  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Changes

When deleting a model using a nested belongs to relation, the model will now be queried and then deleted. This ensures model events to be triggered.

Breaking changes

None.

@spawnia

spawnia commented Jul 19, 2024

Copy link
Copy Markdown
Collaborator

This would probably also apply to @delete when used as an ArgResolver. It also calls $relation->delete() or $related::destroy().

@spawnia spawnia added the enhancement A feature or improvement label Jul 19, 2024
@skaesdorf

skaesdorf commented Jul 19, 2024

Copy link
Copy Markdown
Author

True, but thats just the case for BelongsTo/HasOne. Other cases handled by $related::destroy() are fine since it queries the models. I commited the fix for BelongsTo/HasOne.

@spawnia

spawnia commented Jul 19, 2024

Copy link
Copy Markdown
Collaborator

Would it be simpler to let Laravel handle the fetching by using destroy() in all cases?

@skaesdorf

skaesdorf commented Jul 19, 2024

Copy link
Copy Markdown
Author

I think thats not possible for HasOne since the id of the related model id is unknown. The id at this point is just nested inside a where condition of $relation->query. so, the related model has to be queried first to get the id. destroy() would query the model again.

@skaesdorf

skaesdorf commented Jul 19, 2024

Copy link
Copy Markdown
Author

I just stumbled across an edgecase pitfall of destroy() that maybe should be considered.

destroy() itself is a static method, therefore only global scopes of the model may be respected. When a relation has a scope attached to it like this ...

    public function verifiedUsers () : BelongsTo
    {
        return $this->belongsTo( User::class )->onlyVerified();
    }

... it will not be respected since it is a) lost when calling $related = $relation->make(); and b) lost because destroy() is static. Therefore destroy() might not be a good idea at all? This allows cases where mutations are able to delete models that are not part of their scope.

@skaesdorf

Copy link
Copy Markdown
Author

@spawnia Any thoughts? I would like to solve this.

@skaesdorf

Copy link
Copy Markdown
Author

Hi, can we please merge this?

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

This PR changes nested delete behavior for belongsTo/hasOne-like relationships so the related model is first loaded and then deleted via Eloquent model instance deletion, ensuring model events (e.g. deleting) are fired during nested mutations.

Changes:

  • Update nested delete implementation to call first()?->delete() instead of relation-level delete(), enabling Eloquent model events.
  • Add integration tests asserting deleting events fire for nested deletes in both directive-based and nested-input mutation paths.
  • Minor test setup adjustments to create/associate related models consistently.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
tests/Integration/Schema/Directives/DeleteDirectiveTest.php Adds tests verifying nested @delete(relation: ...) triggers model events; adjusts relation setup.
tests/Integration/Execution/MutationExecutor/BelongsToTest.php Adds tests verifying nested belongsTo delete in input mutations triggers model events; tweaks setup assertions.
src/Schema/Directives/DeleteDirective.php Switches hasOne/belongsTo-like nested delete from relation delete to model-instance delete.
src/Execution/Arguments/NestedBelongsTo.php Switches nested belongsTo delete from relation delete to model-instance delete.

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

Comment on lines 89 to +93
$relation->dissociate();
$relation->getParent()->save();
}

$relation->delete();
$relation->first()?->delete();
Comment on lines 73 to +74
$relation->dissociate();
$relation->delete();
$relation->first()?->delete();
@spawnia

spawnia commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Hi, can we please merge this?

I let Copilot review - I think the comments at least show some gaps in the test cases. I would like to have the behavior nailed down.

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

Labels

enhancement A feature or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants