Framework: Array Cache driver returns variables by reference instead by value

[patriziotomato]

• Laravel Version: v5.5.34
• PHP Version: PHP 7.0.27-0+deb9u1
• Database Driver & Version: Mysql & MariaDB 10.1

Description:

If you're in test environment (or better to say: if you're using the Array Cache driver), the remember() function returns the cached value by reference which can cause unexpected results.

Please see the example below. It is expected, that $cachedCollection in method getFromCache() will return the same value twice, but not a reference to the same memory location.

What I mean is, that in the example below $collection1 and $collection2 are equal. Meaning that if you change $collection2, also $collection1 will change. They point to the same memory.

Will, kind of expected behavior of PHP, but the problem for Laravel is, that other Cache-Drivers like Redis or File will work differently. They do return the same cached value, but not a reference.

This can lead to a very different behavior in your test, compared to your app. The example below shows the piece of code that will cause different results between your app (using redis) and your test (using array driver) because of the described behavior.

Steps To Reproduce:

Write a test like this

    function testWeiredCacheBehaviourWithArrayDriver() {
        $collection1 = $this->getFromCache(4);
        $collection2 = $this->getFromCache(5);

        $this->assertEquals(4, $collection1->last());
    }

    function getFromCache($appendToCache) {
        $cachedCollection = Cache::remember('testkey', 30, function () {
            return new \Illuminate\Support\Collection([1,2,3]);
        });

        return $cachedCollection->push($appendToCache);
    }

Or, if you like, copy this test method to tests/Cache/CacheArrayStoreTest and you will see the test break.

    public function testGettingBackCachedItemsByValue() {
        $cacheKey = 'testKey';
        $store = new ArrayStore;

        $store->put($cacheKey, new \Illuminate\Support\Collection([1,2,3]), 60);

        $collection1 = $store->get($cacheKey)->push(4);
        // Using the same cache key as above, but this should not modify $collection1
        $collection2 = $store->get($cacheKey)->push(5);

        $this->assertEquals(4, $collection1->last());
        $this->assertEquals(5, $collection2->last());
    }

Proposal:

Right now I found no other way than forcing a clone of the object after the cached value has been received like:

    $return = unserialize(serialize($return));

Very ugly of course but I think we should force the Array Cache Driver to ensure to a clone value instead of a variable reference.

Any opinions about that?

[patriziotomato]

See #23082 for a concrete proposal

[NoelDeMartin]

I was going to open an issue with this but I see it has already been reported. Any news on the status? I see that Taylor rejected the PR but since this is still open I guess it's considered a bug and not a feature.

The problem I am facing is that I was caching some models, and in my tests I was asserting that no calls to the database were made. However, turns out that I was using some relations without eager loading, but since models were stored in memory on my tests, the objects had the relations loaded on the second call and my tests were passing. This caused a N+1 issue in production so definitely something to keep in mind!

My solution for now is very similar to @patriziotomato PR so until this is fixed, I'll just use a custom cache driver.

[driesvints]

@NoelDeMartin @patriziotomato is this still an issue for you in the latest version of the framework? The Array Cache driver has gone under quite some changes since.

[NoelDeMartin]

Hi there @driesvints

I can confirm this problem is still happening. I've uploaded a repo that reproduces the issue with two tests: one passing (with a custom array driver that uses serialize) and one failing (with the default array driver).

Here's the failed tests and here's the commit with all the modifications to an out-of-the-box Laravel project.

[driesvints]

Thanks. Still welcoming prs for this.

[NoelDeMartin]

I didn't realize you were accepting PRs, I've opened a new one. I hope it helps :).

[driesvints]

A workaround has been implemented into the future v7 release: https://github.com/laravel/framework/pull/31295