Skip to content
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

[5.8] Fixed cache repository getMultiple implementation #29047

Merged
merged 1 commit into from Jul 5, 2019

Conversation

Projects
None yet
5 participants
@GrahamCampbell
Copy link
Member

commented Jul 2, 2019

The $default param of Psr\SimpleCache\CacheInterface::getMultiple is meant to be the default value, and not an array of defaults, or something of a similar form


For example, Symfony call getMultiple using new stdClass as the defaultm which crashes, because Laravel is trying to iterate over the parameter, instead of using it as the default.

Error Cannot use object of type stdClass as array 
    .../vendor/laravel/framework/src/Illuminate/Cache/Repository.php:131 Illuminate\Cache\Repository::getMultiple
    .../vendor/symfony/cache/Adapter/SimpleCacheAdapter.php:45 Symfony\Component\Cache\Adapter\SimpleCacheAdapter::doFetch
    .../vendor/symfony/cache/Adapter/AbstractAdapter.php:164 Symfony\Component\Cache\Adapter\AbstractAdapter::getItem
    .../vendor/php-http/cache-plugin/src/CachePlugin.php:143 Http\Client\Common\Plugin\CachePlugin::doHandleRequest

@taylorotwell taylorotwell requested a review from driesvints Jul 3, 2019

@driesvints
Copy link
Member

left a comment

You're correct. Good catch 👍

@GrahamCampbell

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

Good catch by Bugsnag when I deployed to production like a fool, then had thousands of errors. 😆

@GrahamCampbell GrahamCampbell force-pushed the 5.8-cache-get-multiple-fix branch from 12e97cf to 67fad0d Jul 4, 2019

@taylorotwell taylorotwell merged commit c4be803 into 5.8 Jul 5, 2019

4 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/styleci/push The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@GrahamCampbell GrahamCampbell deleted the 5.8-cache-get-multiple-fix branch Jul 5, 2019

@atymic

This comment has been minimized.

Copy link

commented Jul 7, 2019

Nice catch! I encountered this as well 😄
Thanks @GrahamCampbell

@crynobone

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

Screen Shot 2019-07-08 at 10 54 34 PM

Been testing the upcoming version with include this changes and Cache::getMultiple() is failing for following example:

Cache::driver('dynamodb')->getMultiple(['key1', 'key2', 'key3'], [
   'foo' => 'bar',
]);

It's as if the default can only be a string, which shouldn't be the case.

@driesvints

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

@crynobone that's very odd. From what I can see in the current implementation on the 5.8 branch that cannot happen. The keys from the first array are used as keys in a new array and every value for each key is the default (which can be an array). Then those keys are filtered as values for the many method on the store. So to me the exception above can't possible be thrown.

Can you perhaps provide a failing test that proves the bug?

@crynobone

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

Can you perhaps provide a failing test that proves the bug?

It's happening for DynamoDbStore, l'm try and have a look again and see if it persist. From the error it seem to chosen default $value instead of $key so it might be the array I'm using doesn't properly cast to string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.