Skip to content

Customisable caching time #434

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

Closed
wants to merge 2 commits into from
Closed

Customisable caching time #434

wants to merge 2 commits into from

Conversation

jm-sky
Copy link

@jm-sky jm-sky commented Jul 28, 2022

This pull request adds two changes:

  • Fixed resolving model in flushModelCache method (because of Class "AppModelCountry" not found error)
  • Added an option to configure caching time
    • to use for all models (global) - in config file
    • to use per model - in $caching_time variable on each model

We can configure caching_time in config file or in .env file (MODEL_CACHE_TIME). If it's provided caching would use remember instead of rememberForever.

Or we can add public $caching_time = 10; property on model.

use GeneaLabs\LaravelModelCaching\Traits\Cachable;

class Country extends Model
{
    use HasFactory, Cachable;

    /** @var integer */
    public $caching_time = 7;

@mikebronner
Copy link
Owner

HI @jm-sky thank you for submitting the PR.

Can you explain how cache-time is different than cooldown, which already exists?

@jm-sky
Copy link
Author

jm-sky commented Jul 28, 2022

I understand that Cache Cool-down delays invalidation of models cache.

My Caching time option adds automatic invalidation after n-seconds.

Basic flow:

  • first CountryModel::all() - query database, set cache for n-seconds
  • next CountryModel::all() - loads result from cache
  • after n-seconds CountryModel::all() - query database, set cache for n-seconds

@Ham3D
Copy link

Ham3D commented Sep 29, 2022

is this PR a good missing feature?

I didn't know until now that, this package cache everything forever(until something change, or we purge it ourselves).
why don't we have a duration config for caching?

@mikebronner
Copy link
Owner

is this PR a good missing feature?

I didn't know until now that, this package cache everything forever(until something change, or we purge it ourselves). why don't we have a duration config for caching?

Yea, I'm not sure this is consistent with the goals of this package. I don't see why you would want to force expiring of cache when it invalidates itself. If there are problems with cached items not getting invalidated, that should be addressed.

@jm-sky
Copy link
Author

jm-sky commented Sep 29, 2022

Thanks anyway. It's useful package.

I've just extended Your classes in my project.

I do so in case of raw inserts into database, that could occur in case of data integration at database level.

@mikebronner
Copy link
Owner

@jm-sky if you do have raw inserts in an automated process, the solution would be to invalidate the cache for the affected models after the script has completed. As a matter of process, if you are scripting the inserts within Laravel (in a console command, or wherever), the model should always be used to perform the insert.

@danpalmieri
Copy link

Is this merged?

@mikebronner
Copy link
Owner

Is this merged?

Hi @danpalmieri, this was not merged, as it goes against the core intent of the package.

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

Successfully merging this pull request may close these issues.

4 participants