GoogleCloudPlatform / appengine-php-sdk

Google App Engine PHP SDK
Apache License 2.0
29 stars 33 forks source link

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

Open p00ya opened 1 year ago

p00ya commented 1 year ago

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:

FYI I'm also a Google SWE, LMK if I should follow up here or internally for contributing.

jama22 commented 1 year ago

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