renoki-co / laravel-eloquent-query-cache

Adding cache on your Laravel Eloquent queries' results is now a breeze.
Apache License 2.0
1.06k stars 118 forks source link

Issues with string params #54

Closed officialmorse closed 3 years ago

officialmorse commented 3 years ago

Hi, firstly let me say I love the package. It Has saved a great deal of time improving efficiency within our app.

While implementing this within my setup I've noticed a couple of issues.

The public function get($columns = ['*']) and public function selectSub($query, $as) methods implemented in Rennokki\QueryCache\Query\Builder handle parameters differently to \Illuminate\Database\Eloquent\Builder

public function get($columns = ['*'])

The method should be able to accept a single column as a string and then wrap this using the Arr::wrap helper as declared in \Illuminate\Database\Eloquent\Builder::get.

When the model does not need caching this is fine as the parent method is called immediately and the string is wrapped. Going through the QueryCacheModule::getFromQueryCache produces the following stack trace

This was previously mentioned in #46

Argument 1 passed to Illuminate\Database\Grammar::columnize() must be of the type array, string given, called in D:\xampp\htdocs\j2e_laravel\vendor\laravel\framework\src\Illuminate\Database\Query\Grammars\Grammar.php on line 143 {"exception":"[object] (TypeError(code: 0): Argument 1 passed to Illuminate\\Database\\Grammar::columnize() must be of the type array, string given, called in D:\\xampp\\htdocs\\j2e_laravel\\vendor\\laravel\\framework\\src\\Illuminate\\Database\\Query\\Grammars\\Grammar.php on line 143 at D:\\xampp\\htdocs\\j2e_laravel\\vendor\\laravel\\framework\\src\\Illuminate\\Database\\Grammar.php:125)
[stacktrace]
#0 D:\\xampp\\htdocs\\j2e_laravel\\vendor\\laravel\\framework\\src\\Illuminate\\Database\\Query\\Grammars\\Grammar.php(143): Illuminate\\Database\\Grammar->columnize('homework_id')
#1 D:\\xampp\\htdocs\\j2e_laravel\\vendor\\laravel\\framework\\src\\Illuminate\\Database\\Query\\Grammars\\Grammar.php(91): Illuminate\\Database\\Query\\Grammars\\Grammar->compileColumns(Object(Rennokki\\QueryCache\\Query\\Builder), 'homework_id')
#2 D:\\xampp\\htdocs\\j2e_laravel\\vendor\\laravel\\framework\\src\\Illuminate\\Database\\Query\\Grammars\\Grammar.php(65): Illuminate\\Database\\Query\\Grammars\\Grammar->compileComponents(Object(Rennokki\\QueryCache\\Query\\Builder))
#3 D:\\xampp\\htdocs\\j2e_laravel\\vendor\\laravel\\framework\\src\\Illuminate\\Database\\Query\\Builder.php(2264): Illuminate\\Database\\Query\\Grammars\\Grammar->compileSelect(Object(Rennokki\\QueryCache\\Query\\Builder))
#4 D:\\xampp\\htdocs\\j2e_laravel\\vendor\
ennokki\\laravel-eloquent-query-cache\\src\\Traits\\QueryCacheModule.php(158): Illuminate\\Database\\Query\\Builder->toSql()
#5 D:\\xampp\\htdocs\\j2e_laravel\\vendor\
ennokki\\laravel-eloquent-query-cache\\src\\Traits\\QueryCacheModule.php(132): Rennokki\\QueryCache\\Query\\Builder->generatePlainCacheKey('get', NULL, NULL)
#6 D:\\xampp\\htdocs\\j2e_laravel\\vendor\
ennokki\\laravel-eloquent-query-cache\\src\\Traits\\QueryCacheModule.php(116): Rennokki\\QueryCache\\Query\\Builder->generateCacheKey('get', NULL, NULL)
#7 D:\\xampp\\htdocs\\j2e_laravel\\vendor\
ennokki\\laravel-eloquent-query-cache\\src\\Traits\\QueryCacheModule.php(77): Rennokki\\QueryCache\\Query\\Builder->getCacheKey('get')
#8 D:\\xampp\\htdocs\\j2e_laravel\\vendor\
ennokki\\laravel-eloquent-query-cache\\src\\Query\\Builder.php(20): Rennokki\\QueryCache\\Query\\Builder->getFromQueryCache('get', 'homework_id')
#9 D:\\xampp\\htdocs\\j2e_laravel\\vendor\\laravel\\framework\\src\\Illuminate\\Database\\Eloquent\\Builder.php(571): Rennokki\\QueryCache\\Query\\Builder->get('homework_id')
#10 D:\\xampp\\htdocs\\j2e_laravel\\vendor\\laravel\\framework\\src\\Illuminate\\Database\\Eloquent\\Builder.php(555): Illuminate\\Database\\Eloquent\\Builder->getModels('homework_id')
#11 D:\\xampp\\htdocs\\j2e_laravel\\app\\Listeners\\Homework\\ClearUserOldClassNotifications.php(41): Illuminate\\Database\\Eloquent\\Builder->get('homework_id')

public function selectSub($query, $as)

When passing a raw query as a string, the if statement used to check whether to append cache tags produces an error as the get_class is expecting an object to be passed.

local.ERROR: get_class() expects parameter 1 to be object, string given {"userId":22,"exception":"[object] (ErrorException(code: 0): get_class() expects parameter 1 to be object, string given at D:\\xampp\\htdocs\\j2e_laravel\\vendor\
ennokki\\laravel-eloquent-query-cache\\src\\Query\\Builder.php:49)
[stacktrace]
#0 [internal function]: Illuminate\\Foundation\\Bootstrap\\HandleExceptions->handleError(2, 'get_class() exp...', 'D:\\\\xampp\\\\htdocs...', 49, Array)
#1 D:\\xampp\\htdocs\\j2e_laravel\\vendor\
ennokki\\laravel-eloquent-query-cache\\src\\Query\\Builder.php(49): get_class('SELECT `date` F...')
#2 D:\\xampp\\htdocs\\j2e_laravel\\vendor\\laravel\\framework\\src\\Illuminate\\Support\\Traits\\ForwardsCalls.php(23): Rennokki\\QueryCache\\Query\\Builder->selectSub('SELECT `date` F...', 'last_login')
#3 D:\\xampp\\htdocs\\j2e_laravel\\vendor\\laravel\\framework\\src\\Illuminate\\Database\\Eloquent\\Builder.php(1533): Illuminate\\Database\\Eloquent\\Builder->forwardCallTo(Object(Rennokki\\QueryCache\\Query\\Builder), 'selectSub', Array)
#4 D:\\xampp\\htdocs\\j2e_laravel\\app\\Http\\Controllers\\Management\\SearchController.php(75): Illuminate\\Database\\Eloquent\\Builder->__call('selectSub', Array)

If you're happy for me to submit a PR I can make the necessary changes.

Thanks, Luke

joshuacashcalc commented 3 years ago

Any movement on this? I have a very similar issue.

rennokki commented 3 years ago

@officialmorse @joshuacashcalc Laravel versions?

joshuacashcalc commented 3 years ago

I'm using laravel 6.0 and another project on laravel 8

officialmorse commented 3 years ago

@officialmorse @joshuacashcalc Laravel versions?

v8.24.0

rennokki commented 3 years ago

@joshuacashcalc What version is bringing problems?

joshuacashcalc commented 3 years ago

Sorry I should have been more specific. Laravel 8.26.1 and the latest version of this package. Laravel 6 has nothing to do with this project, apologies!

rennokki commented 3 years ago

That should be fixed.

Unfortunately, raw queries will not be cached using ->selectSub()