Framework: Memcached store producing an error with the "many" method

[nikkiii]

Version: 5.2.29
Interpreter: HHVM 3.12.1, PHP 5.6, PHP 7.0
Error:
Fatal error: Cannot pass parameter 2 by reference in /var/www/vendor/laravel/framework/src/Illuminate/Cache/MemcachedStore.php on line 66

When using getMulti in the Illuminate\Cache\MemcachedStore class, the second argument is an incorrectly passed reference.

To reproduce:

• Set cache store to Memcached
• Call the many method on the store

app('cache')->store('memcached')->many([ 'test1', 'test2' ]);

Solution:
Change null to $null, assign $null = null above. The Memcached.getMulti documentation does just this in this example.

I'm not sure if this affects PHP too, or just HHVM, but it should appear in both.

[GrahamCampbell]

Ping @taylorotwell.

[GrahamCampbell]

I'm not sure if this affects PHP too, or just HHVM, but it should appear in both.

Could you please check @nikkiii.

[nikkiii]

@GrahamCampbell Tested on PHP 5.6, it doesn't cause a fatal error and returns as expected. Not worth an immediate patch release as it's an extremely rare edge case in my opinion, but it would be nice to see a patch combined with other bugfixes/versions none the less.

[GrahamCampbell]

Please report this error to HHVM.

[nikkiii]

Actually, seems to be affecting PHP, even then it's not specifically an HHVM issue since the cause is an incorrect reference, and strict checking on this doesn't make it something that they need to fix. I'm also not sure if my test correctly covered the issue when using PHP 5.6, as I'm not sure how type checking on extensions works, but it seems like it doesn't catch this error.

PHP's documentation states that references cannot be primitives/etc, and attempting to pass null/an integer causes it to fail.

This is confirmed by a basic script:

<?php

some_reference(null);
some_reference(1234);

function some_reference(&$var) {
    // nothing
}

Which results in this, for both calls:

PHP Fatal error: Uncaught Error: Cannot pass parameter 1 by reference

[nikkiii]

@GrahamCampbell Any chance this issue can be re-opened? Not an HHVM issue, but definitely a Laravel one.

[themsaid]

Closing since Laravel 5.3 doesn't support hhvm and 5.2 is closed.

[sisve]

@themsaid Ignore the HHVM-part; the docs for getMulti indicates it's by-reference, and you cannot pass null directly to it. It must go via a variable, like the example code shows. The issue fails on PHP 5.6 too.

Line: https://github.com/laravel/framework/blob/5.3/src/Illuminate/Cache/MemcachedStore.php#L67
Docs: http://php.net/manual/en/memcached.getmulti.php
3v4l example: https://3v4l.org/b342d

[sisve]

After a lot of tests it seems to end up as a case of php magic. The internal api (getMulti) allows these kind of things, while userland code doesn't. So ignore my comment. It never happened. _does jedi hand-wave_

[nikkiii]

It seems to depend on the Memcached library version, I'm currently looking into it to see where the change causing it to pass/fail is.

[sisve]

To add to the fun; the current code will be invalid in the latest release of memcached since the method signature recently. Wordpress has a related issue created only 8 days ago.

Issue: https://github.com/php-memcached-dev/php-memcached/issues/229
Wordpress issue: https://github.com/tollmanz/wordpress-pecl-memcached-object-cache/issues/86

[lagbox]

Much fun, for me that method signature is different, at least the PHP 7 version of this. Seems there are different signatures in the different branches of this extension.

Cache::many(['key1', 'k2']);

PHP warning: Memcached::getMulti() expects at most 2 parameters, 3 given in /var/www/html/sites/l53/vendor/laravel/framework/src/Illuminate/Cache/MemcachedStore.php on line 66