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

Add replicating and pruning event handlers #149

Closed
wants to merge 3 commits into from

Conversation

marvindurot
Copy link

This PR adds nice event handlers for replicating and pruning Laravel events in the Storm database model.

See:

@LukeTowers
Copy link
Member

@marvindurot are there replicated and pruned events too?

@marvindurot
Copy link
Author

@marvindurot are there replicated and pruned events too?

No, these events don't exist, which is why I didn't add them.

src/Database/Model.php Outdated Show resolved Hide resolved
src/Database/Model.php Outdated Show resolved Hide resolved
@LukeTowers
Copy link
Member

@bennothommo Could you look at this? I'm not seeing anything obvious that would be causing the tests to fail

@bennothommo
Copy link
Member

bennothommo commented Jul 23, 2023

@LukeTowers @marvindurot the issue is that Laravel's base model doesn't have a pruning method. The method is provided by the Prunable trait.

This means that in our model, when it boots the "nicer" events and hits "beforePrune", the call to pruning is passed to the __call magic method which tries to feed it through to a scope method, or failing that, tries to pass it to an extension. When it tries to do it to the extensions, it's trying to load the default database configuration which doesn't exist on our test copy so that causes the test to fail.

We have three options:

  • We can fix the test environment to have a default config (I would rather not do this, because we want the tests to run without a database needed)
  • We can extend Laravel's Prunable trait to add our own events where needed
  • We can add a pruning method to our base model (seems kinda dirty if it's not needed unless the Prunable trait is used)

@github-actions
Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this is still being worked on, please respond and we will re-open this pull request.
If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

@LukeTowers
Copy link
Member

@marvindurot do you have any thoughts on what @bennothommo said?

Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in the last 60 days.
If this is still being worked on, please respond and we will re-open this pull request.
If this pull request is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

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

Successfully merging this pull request may close these issues.

3 participants