Skip to content
This repository has been archived by the owner on Sep 20, 2019. It is now read-only.

Don't require Eloquent for storing and retrieving of events #183

Merged
merged 25 commits into from
Sep 4, 2019

Conversation

riasvdv
Copy link
Member

@riasvdv riasvdv commented Aug 9, 2019

This refactor / implementation allows you to have a different storage model other than Eloquent. For example, a simple QueryStoredEventRepository that runs mysql queries would be far more performant on millions of events than building Eloquent models for them.

This PR does the following things:

  • Adds a stored_event_repository key in the config
  • An event repository must implement the StoredEventRepository interface
  • All internal model calls have been refactored to a StoredEventData model that has the necessary data
  • An EloquentStoredEventRepository has been added to keep the default implementation, the StoredEvent model is now only used in this specific implementation, custom StoredEvent models still need to extend this.
  • An OtherEloquentStoredEventRepository is added in the tests to test the implementation

Upgrade guide

  • Remove the stored_event_model config key from your event-projector.php config file
  • Add a stored_event_repository config key with the following value: \Spatie\EventProjector\EloquentStoredEventRepository::class
  • If you're using a different model for event storage:
    1. The \Spatie\EventProjector\Models\StoredEvent::class has been renamed to \Spatie\EventProjector\Models\EloquentStoredEvent::class
    2. Extend the \Spatie\EventProjector\EloquentStoredEventRepository and change the $storedEventModel property with your own model that extends \Spatie\EventProjector\Models\EloquentStoredEvent
    3. Change the stored_event_repository config value with your own repository

@riasvdv riasvdv requested a review from freekmurze August 9, 2019 13:39
@riasvdv
Copy link
Member Author

riasvdv commented Aug 9, 2019

Would be great if this could be tested in an existing project that uses Event Projector before we merge this, although the tests seem pretty complete, this is a pretty drastic change.

@freekmurze freekmurze self-assigned this Aug 9, 2019
src/StoredEventRepository.php Outdated Show resolved Hide resolved
src/EloquentStoredEventRepository.php Outdated Show resolved Hide resolved
src/Models/StoredEventData.php Outdated Show resolved Hide resolved
@freekmurze
Copy link
Member

freekmurze commented Aug 13, 2019 via email

@riasvdv riasvdv changed the title Allow Events to be stored somewhere other than eloquent Don't require Eloquent for storing and retrieving of events Aug 16, 2019
@Gummibeer
Copy link

💙🎉👍 14eeb4a

@freekmurze freekmurze marked this pull request as ready for review August 22, 2019 06:44
Copy link
Member

@freekmurze freekmurze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already looks really good 👍

The reason why we do this change is to make the package a bit more flexible when storing a lot of events. With the current changes, users with a large event store might still run into trouble. We need to fix this.

src/Console/ReplayCommand.php Outdated Show resolved Hide resolved
src/StoredEventRepository.php Outdated Show resolved Hide resolved
@freekmurze
Copy link
Member

@riasvdv Could you also test this out by updating the larabank apps?
https://github.com/spatie/larabank-event-projector
https://github.com/spatie/larabank-event-projector-aggregates

Just use dev-master of this package in those app. Feel free to make commits on the master branch of the apps.

composer.json Outdated Show resolved Hide resolved
@riasvdv
Copy link
Member Author

riasvdv commented Sep 4, 2019

Just tested both Larabank applications with the version in this branch, all functionality still works 🎉

@freekmurze Could you go over this one more time? Then we can tag v3

@freekmurze freekmurze merged commit 0de009e into master Sep 4, 2019
@freekmurze freekmurze deleted the stored-event-repository branch September 4, 2019 09:41
@freekmurze
Copy link
Member

Epic PR, thanks @riasvdv

@sebastiandedeyne
Copy link
Member

💪 💪

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

Successfully merging this pull request may close these issues.

None yet

4 participants