GoogleCloudPlatform / GoogleCloudPlatform/appengine-php-sdk

Memcache::get should set its $flags parameter to an int

Open
#105 1 comment 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.