Skip to content

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

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

Conversation

thyseus
Copy link

@thyseus thyseus commented Mar 16, 2021

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
Copy link
Owner

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
Copy link
Author

thyseus commented Mar 16, 2021

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",
		];
  • which part in $key does represent the tablename ?

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
Copy link
Owner

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
Copy link
Author

thyseus commented Mar 16, 2021

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

@mikebronner mikebronner merged commit ba18569 into mikebronner:master Mar 16, 2021
@mikebronner
Copy link
Owner

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

@thyseus thyseus deleted the caching-key-query-from-instead-model-table branch March 16, 2021 15:14
@thyseus
Copy link
Author

thyseus commented Mar 16, 2021

@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
Copy link
Owner

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

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.

2 participants