Skip to content
This repository has been archived by the owner. It is now read-only.

Delete via model instead of QueryBuilder #361

Merged
merged 2 commits into from Jun 7, 2017

Conversation

@andersLAL
Copy link
Contributor

commented Jun 7, 2017

I use Translatable along with a model logging package that uses events to log changes. Deleting translations did not log (deleted event didn't fire) because they were deleted directly from the QueryBuilder, not model instances.

andersLAL added some commits Jun 7, 2017

Delete via model instead of Builder
In order to fire deleted events, delete via the model instance instead of directly on the Query/Builder.

@dimsav dimsav merged commit 1e520ca into dimsav:master Jun 7, 2017

1 check passed

continuous-integration/styleci/pr The StyleCI analysis has passed
Details
@dimsav

This comment has been minimized.

Copy link
Owner

commented Jun 7, 2017

Thank you @andersLAL!

@dimsav

This comment has been minimized.

Copy link
Owner

commented Jun 7, 2017

Fix was just released. :-)

@andersLAL

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2017

Thanks :)

@Gummibeer

This comment has been minimized.

Copy link
Collaborator

commented Jun 8, 2017

Ok, but this is a massive Performance killer. Instead of one query it runs now 1+n in a foreach. And just to have events, which will decrease Performance also, is not a simple decision. :/

@dimsav

This comment has been minimized.

Copy link
Owner

commented Jun 8, 2017

Thank you @Gummibeer. Should we make it optional via configuration?

@Gummibeer

This comment has been minimized.

Copy link
Collaborator

commented Jun 8, 2017

@dimsav I think there are some options:

  1. Write it in readme to let the users who need it override it by themself
  2. Add a second Trait that does the first point
  3. Add a configuration option to decide what to do
  4. Send a custom event after the query delete

But adding a 1+n queries instead of 1 query without alternative is a downgrade for me. So there should be a way to get around.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.