appengine-php-sdk icon indicating copy to clipboard operation
appengine-php-sdk copied to clipboard

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

Open p00ya opened this issue 2 years ago • 1 comments

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(). 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.

p00ya avatar Apr 30 '23 03:04 p00ya

Sounds like a reasonable request. If you're looking to contribute the change we'd be more than happy to help review

jama22 avatar May 16 '23 21:05 jama22