CacheBundle icon indicating copy to clipboard operation
CacheBundle copied to clipboard

Multiget

Open e120281 opened this issue 12 years ago • 9 comments

Hi Jeremy,

I thought it would be nice to be able to take advantage of memcached multiget functionality. In current implementation multiget (I give array of keys to get function) fails because of "$key = $this->prefix . $key;" line.

public function get($key)
{
    if ($this->isSafe()) {
        $key = $this->prefix . $key;
        return $this->mem->get($key);
    }

    return false;
}

Something like below should do the work, I guess. Please let me know what you think.

public function get($key)
{
    if ($this->isSafe()) {
        if (is_array($key)) {
            if ($this->prefix != '') {
                for ($i = 0; $i < count($key); $i++) {
                    $key[$i] = $this->prefix . $key[$i];
                }
            }
        }
        else {
            $key = $this->prefix . $key;
        }

        return $this->mem->get($key);
    }

    return false;
}

e120281 avatar Aug 11 '13 18:08 e120281

Is there a reason why you're checking to see if the prefix isn't an empty string in the array situation?

jeremylivingston avatar Aug 15 '13 00:08 jeremylivingston

Took me a second to figure it out, but: In a situation with no prefix, the raw array is passed to get(); when there is a prefix, the array is modified so all the individual keys are assigned the prefix before being passed to get().

beryllium avatar Aug 15 '13 01:08 beryllium

Ah, you're right. I think we could apply this same behavior when the key is a string. I can restructure this code to make it a little clearer.

@beryllium, are you okay with this change?

jeremylivingston avatar Aug 15 '13 01:08 jeremylivingston

Mostly - but I would prefer to use a foreach($key as $index=>$value) instead of for() loop, this might be safer for handling non-numeric array keys:

foreach($key as $index=>$value)
{
    $key[$index] = $this->prefix . $value;
}

We might also want to ignore or unset array elements that are empty strings, although I'm not 100% certain if that is necessary or desired:

foreach($key as $index=>$value)
{
    if (empty($value)) 
    {
        unset($key[$index]);
        continue;
    }

    $key[$index] = $this->prefix . $value;
}

beryllium avatar Aug 15 '13 01:08 beryllium

I think that Memcache would simply ignore empty strings, wouldn't it? If we want to prevent these from being passed, we should probably also prevent it for the string case too. Maybe I can add the array functionality for this phase, then validate keys as a follow up.

jeremylivingston avatar Aug 15 '13 02:08 jeremylivingston

Sounds good. Key validation quirks would be a good thing for travis/unit tests, eh? :)

beryllium avatar Aug 15 '13 02:08 beryllium

After I started working toward a solution, I realized that there could be an issue with this.

When you call memcache::get() with an array of keys, it will return an associative array with the keys and their values. If you use a prefix for your client, this array will contain the prefix in each of those keys.

Should these be stripped out before they're returned? Otherwise, you're going to have to be aware of the keys in every place that you retrieve an array from the cache. I'm leaning towards yes, but wanted to run it by you.

jeremylivingston avatar Aug 15 '13 02:08 jeremylivingston

Yes, I think they should be stripped out. Seems to make the most sense that way, with the prefix acting like a namespace.

beryllium avatar Aug 15 '13 02:08 beryllium

I added the functionality to the get() method. Please take a look when you have a moment.

jeremylivingston avatar Aug 15 '13 03:08 jeremylivingston