GoogleCloudPlatform / GoogleCloudPlatform/appengine-php-sdk
Memcache::get should set its $flags parameter to an int
- Dominant language
- PHP
- Stars
- 29
- Forks
- 36
- PR merge metrics
- No merged PRs in 30d
Description
From https://www.php.net/manual/en/memcache.get.php:
> If present, flags fetched along with the values will be written to this parameter. These flags are the same as the ones given to for example [Memcache::set()](https://www.php.net/manual/en/memcache.set.php). The lowest byte of the int is reserved for pecl/memcache internal usage (e.g. to indicate compression and serialization status).
However, in the App Engine SDK the `$flags` parameter of `Memcache::get` is not a reference parameter: https://github.com/GoogleCloudPlatform/appengine-php-sdk/blob/master/src/Api/Memcache/Memcache.php#L211
```
public function get($keys, $flags = null) {
```
Some Memcache clients rely on the PECL behaviour of setting $flags to an int to indicate a successful lookup. This is the only way the PECL API provides to distinguish a cache hit on a `false` value from a cache miss. An example is https://github.com/Automattic/wp-memcached/blob/4.0.0/object-cache.php#L432-L441:
```
$flags = false;
$this->timer_start();
$value = $mc->get( $key, $flags );
$elapsed = $this->timer_stop();
// Value will be unchanged if the key doesn't exist.
if ( false === $flags ) {
$found = false;
$value = false;
}
```
I think App Engine's `Memcache::get()` should set the `$flags`, using the value from `$item->getFlags()` at https://github.com/GoogleCloudPlatform/appengine-php-sdk/blob/master/src/Api/Memcache/Memcache.php#L190. Some thoughts though:
* It will be a bit weird that Memcache returns `$flags` on `get` but ignore `$flag` on `set`. But this isn't a regression, and it doesn't even seem that asymmetric to me (App Engine will continue not to support client-set flags).
* This will expose some internals, which clients shouldn't rely on (other than `$flags` being overwritten by an int). But this is similar to the PECL module, whose documentation states "the lowest byte of the int is reserved for pecl/memcache internal usage (e.g. to indicate compression and serialization status)". Alternatively, it could set `$flags` to a constant like 0 on lookup success.
FYI I'm also a Google SWE, LMK if I should follow up here or internally for contributing.
Contributor guide
Assessment
This issue has not been assessed yet.