tpetry / laravel-postgresql-enhanced

Support for many missing PostgreSQL specific features
MIT License
772 stars 31 forks source link

feat: support deleteReturning in RefreshDataOnSave trait #22

Closed jaulz closed 2 years ago

jaulz commented 2 years ago

I have a use case where I use a trigger to prevent the deletion of a row in case a certain condition is met. This PR enhances the RefreshDataOnSave trait in way that it uses the existing deleteReturning functionality and only marks the instance as deleted in case it was really deleted. Please let me know what you think and in case you would consider it I can also implement a corresponding test.

tpetry commented 2 years ago

Can you describe more fully your use case and why you believe the trait should change the delete behaviour? I am very hesitant changing a core behaviour as it may break some application which rely on the other behaviour.

jaulz commented 2 years ago

It's not necessarily that we override the core behaviour, we could also add it as part of another method, e.g. tryDelete. I just thought that it might be handy since there is anyway the deleteReturning and that would actually make use of it 😊 In my scenario I don't want primary emails to be ever deleted (i.e. I don't trust the app 😉 ) so the database prevents it by checking the primary flag in a before trigger.

tpetry commented 2 years ago

Thank you again for your input, Julian. I highly appreciate any input! But this time, I will not merge the pull request.

I believe that this is a particular requirement related to your application which not many other applications will share. The best place for this modification will be a trait within your application. Also, the behavior of Eloquent models is changed for the exists property, possibly leading to breaking some applications. I try to extend Laravel as much as possible without breaking any existing code, which can not be guaranteed with this change.