mikebronner / laravel-model-caching

Eloquent model-caching made easy.
MIT License
2.23k stars 212 forks source link

Use the part of the query (->from) instead of the table name of the model (->table) #393

Closed thyseus closed 3 years ago

thyseus commented 3 years ago

Why ?

I have the following model:

class DemoUnscoped {
        public $table = 'demo_unscoped';

    public function scopeFooBar($query)
    {
        $query->from('demo_scoped');

        return $query;
    }
}

Now, assuming my table demo_unscoped has 666 entries, and demo_scoped has 999 entries, prior to this patch i get the following results:

DemoUnscoped::count();
666
DemoUnscoped::FooBar()->count();
666 // <----------- WRONG (has cached the value but should not!)

With this fix applied i get:

DemoUnscoped::count();
666
DemoUnscoped::FooBar()->count();
999 // <----------- Correct ! 
mikebronner commented 3 years ago

Hi @thyseus, thanks for submitting this. Unfortunately the tests aren't passing. Could you take a look into that? Be sure to run the tests locally on your machine and make sure they all pass. I'll try to take a look at it when I have some free time. Thanks! :)

thyseus commented 3 years ago

Hey, @mikebronner - thanks a lot for your quick reply !

I already begun digging into this issue. It seems like i need to adjust the $key fixtures in (every??) test so the tests are using the new source. Unfortunately i can not grasp where the "table/from" part is inside the key:

    $key = sha1("genealabs:laravel-model-caching:testing:{$this->testingSqlitePath}testing.sqlite:book-store:genealabslaravelmodelcachingcachedbelongstomany-book_store.book_id_=_{$bookId}");
    $tags = [
        "genealabs:laravel-model-caching:testing:{$this->testingSqlitePath}testing.sqlite:genealabslaravelmodelcachingtestsfixturesstore",
        ];

Could you give me a little advice ? You know the code much better than me :)

and by the way: i have never seen a such thorougly tested module in my whole life. really good work !!!

mikebronner commented 3 years ago

Ah, yes!!! It's annoying and tedious :) Unfortunately your change seems that it could be affecting all cache keys. Let me take a look as to why they are changing. Most tests should not really change, unless from() is not set in the query. I also need to add PHP 8.0 to the test matrix.

thyseus commented 3 years ago

Thanks a very lot ! Your module is awesome. Let me know if i can be of any help...

mikebronner commented 3 years ago

@thyseus try release 0.11.2 and let me know how it works for you. :) Thanks for submitting this PR! :)

thyseus commented 3 years ago

@mikebronner just did run composer update, geneolabs/laravel-model-caching got updated and everything is working as expected ! Thanks again a lot for this really quick reaction.

mikebronner commented 3 years ago

@thyseus That's awesome! :) Super glad you are enjoying this package.