Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return value for get with callback and Memcached::GET_EXTENDED flag in php7 version #248

Open
tacovandenbroek opened this issue May 10, 2016 · 3 comments

Comments

@tacovandenbroek
Copy link

Using the Memcached::GET_EXTENDED flag, what should be the return value for Memcached::get when a callback is provided and the value is not found in memcached? I would expect a structure like a known value would return, but only the return value of the callback seems to be returned, is this intentional?

@sodabrew
Copy link
Contributor

I gave a read through #126 and #229 and worked on adding some unit tests for the case of get(key, callback, GET_EXTENDED) and I am stuck on not understanding something:

Why in s_invoke_cache_callback is the fourth argument for expiration not passed to the callback?

        if (with_cas) {
                fci->param_count = 3;
        } else {
                ZVAL_NEW_EMPTY_REF(&params[3]);    /* expiration */
                ZVAL_NULL(Z_REFVAL(params[3]));
                fci->param_count = 4;
        }

I don't get it. In GET_EXTENDED mode, the callback has three arguments, where the $value argument is interpreted as an array with keys "value" and "cas" but expiration is nowhere to be found, neither in the callback signature nor in the value array.

@mkoppanen @laruence Could you provide some insight into the design goals on this callback, and whether I should fix/straighten-out this logic by either adding the expirating to the value array, or restoring the fourth argument for expiration from the callback?

I strongly prefer to add back the expiration argument and always have the same number of arguments to the callback in GET_EXTENDED mode and regular mode. I think it's bad enough to change the interpretation of the $value argument as mixed vs. array, but also changing the number of expected arguments feels very very awkward to me.

It is clearly possible to set the expiration on an item with cas, for example: https://secure.php.net/manual/en/memcached.cas.php

@sodabrew
Copy link
Contributor

Also, I notice the following discrepancy:

var_dump (
$m->get ($first_key, function (Memcached $memc, $key, &$value) {
                                        $value = [
                                                "value" => "first_ext",
                                                "cas"   => 12345,
                                                "expiration" => 10 ];
                                        return true;
                                }, Memcached::GET_EXTENDED)
);

var_dump ($m->get ($first_key, null, Memcached::GET_EXTENDED));

Yields:

array(3) {
  ["value"]=>
  string(9) "first_ext"
  ["cas"]=>
  int(%d)
  ["expiration"]=>
  int(10)
}
array(3) {
  ["value"]=>
  string(9) "first_ext"
  ["cas"]=>
  int(%d)
  ["flags"]=>
  int(0)
}

Note how the first array has "expiration", which came from the callback's $value array and is not a normal part of the GET_EXTENDED response array, but is missing "flags", while the second array does have "flags", which is the expected result.

@sodabrew
Copy link
Contributor

I got a little bit stuck. The memcached binary protocol will return the initial cas value from a set-value operation, but the libmemcahched api does not appear to expose it.

@sodabrew sodabrew removed this from the 3.1.0 milestone Jan 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants