Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

skaesdorf
Copy link
Contributor

@skaesdorf skaesdorf commented Jul 18, 2024

  • 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
Copy link
Collaborator

spawnia commented Jul 19, 2024

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
Copy link
Contributor Author

skaesdorf commented Jul 19, 2024

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
Copy link
Collaborator

spawnia commented Jul 19, 2024

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

@skaesdorf
Copy link
Contributor Author

skaesdorf commented Jul 19, 2024

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
Copy link
Contributor Author

skaesdorf commented Jul 19, 2024

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
Contributor Author

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

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.

2 participants