kirkbushell / eloquence

A drop-in library for certain database functionality in Laravel, that allows for extra features that may never make it into the main project.
MIT License
537 stars 58 forks source link

Ability to recalculate a count cache (instead of just incrementing/decrementing) #33

Closed cviebrock closed 8 years ago

cviebrock commented 8 years ago

I can't think of a use case off the top of my head, except for what I just did when I added cache counting to an existing set of models and relations.

The behaviour only calculates if the count should be incremented or decremented from the current value, but there is no way to tell it to do a "hard" recalculation of the value (i.e. get $model->relation()->count() and store it in $model->count_field).

I'm not sure how that could be handled (maybe an artisan command?), or if there should be an option in the countCaches() configuration to tell it to do full recalculations on updates instead of simple increments/decrements.

Thoughts?

kirkbushell commented 8 years ago

It's certainly an option and probably something that may come in handy for some developers - either adding counts to existing models, or maybe you want to change the field or something. I can see several use-cases where this would be necessary.

This could be done via an artisan command that could update all registered count caches maybe? Optionally, a specific model could be provided as part of the command and just that model's counts are updated?

cviebrock commented 8 years ago

eloquence:update-caches with an optional --class=User argument makes sense to me.

If I get a chance, I'll try and put together a PR for you. :)

kirkbushell commented 8 years ago

That would be awesome, thanks :)

I'll help out with anything as well - lemme know if you get stuck or want to run through some ideas.

kirkbushell commented 8 years ago

@cviebrock if you'er interested, I'd love some help on this while I push forward with version 2 of Eloquence :)

cviebrock commented 8 years ago

I'll try and whip something up in the next day or so.

kirkbushell commented 8 years ago

@cviebrock all the updates are now available on the master branch - so feel free to work on that and test if that suits you. Readme/guide also updated to reflect new API and approach to setting up these behaviours.

cviebrock commented 8 years ago

:+1:

cviebrock commented 8 years ago

So, I've almost got this working. The biggest issue is how to find all the Models that use caching, in order to run the command on them.

I'm using https://github.com/hanneskod/classtools to do the searching. However, because you changed the way you define the caching (traits instead of interfaces), I can't easily filter classes via things like instance of CountCache.

Would you consider adding the interfaces back in? I can work around it, for sure, but it would make things easier.

kirkbushell commented 8 years ago

@cviebrock I won't add an interface simply to be included as a marker for a console command.

My suggestion is that the user be required to provide the model(s) that they wish to update the counts for as part of the options for the command.

cviebrock commented 8 years ago

Also ... I think with the reorganization, you can't now have a model that uses both Countable and Summable (because both traits implement the updateCache() and default() methods.

cviebrock commented 8 years ago

I'm offline for a bit, but feel free to check out the "update-caches-command" branch on my fork of your package to see what I have so far.

kirkbushell commented 8 years ago

Ah crud, I better check that. Thanks :)

kirkbushell commented 8 years ago

Any method conflicts should now be resolved. I want to spend a little time cleaning it up further, as I believe there's an opportunity there for some sound refactoring.

cviebrock commented 8 years ago

Well ... Countable uses Cacheable. And Summable uses Cacheable. So you still can't use both because all the methods in Cacheable are trying to get included twice.

Just a suggestion: move all the logic out of the models and into a service class? All that would remain in the model is use Countable, the countCaches() method to set the configuration, and bootCountable() to call the service class on the events.

kirkbushell commented 8 years ago

Ah good point, I didn't think about that. Yeah good idea about the boot aspect managing the service class. Will have a play with that tonight :)

kirkbushell commented 8 years ago

I've refactored that code out into a couple of separate services now (good idea!) and that cleans the traits themselves up quite a bit. Tests are all still passing, so happy to move forward with that if you feel that helps?

cviebrock commented 8 years ago

Cool! I'll pull down your master, and add in my Artisan commands to recalculate cache counts, then submit a PR.

cviebrock commented 8 years ago

Closing this, as the feature is now added.