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

Multiget #16

Open
e120281 opened this issue Aug 11, 2013 · 9 comments
Open

Multiget #16

e120281 opened this issue Aug 11, 2013 · 9 comments

Comments

@e120281
Copy link

e120281 commented Aug 11, 2013

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;
}
@jeremylivingston
Copy link
Collaborator

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

@beryllium
Copy link
Owner

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

@jeremylivingston
Copy link
Collaborator

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?

@beryllium
Copy link
Owner

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;
}

@jeremylivingston
Copy link
Collaborator

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.

@beryllium
Copy link
Owner

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

@jeremylivingston
Copy link
Collaborator

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.

@beryllium
Copy link
Owner

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

@jeremylivingston
Copy link
Collaborator

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

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

3 participants