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

fixed arguments keys on MultipleGet Command #4

Closed
wants to merge 1 commit into from

Conversation

Peace-N
Copy link

@Peace-N Peace-N commented Apr 11, 2019

There is an issue on the MultipleGet command class, we have to shift the arguments by one up the same way we pop the path argument ..

@mkorkmaz
Copy link
Owner

Hi @Peace-N

mget needs an array of keys. This change uses only one key of the desired keys.

Also $keys = array_shift($arguments); returns only a key and it is string, internal mget command you chnaged needs an array.

Usage example of mget is like that:

Say we need to fetch n keys.

$rejsonClient->mget('key_1', 'key_2', ..., 'key_n', '_path_');

The expression in the documentation is ...$keys means there is a number of arguments, it is called as "variadic parameter" and it fetches arguments as an array.

What was your intent?

@Peace-N
Copy link
Author

Peace-N commented Apr 12, 2019

SO Ok we finally managed to get it to work, we were passing dynamic arguments from the db and i think the issue was on unpacking the arguments and also add the path as the last argument element.
Here is a wrapper function we created and used ..

public function mget(array $args, string $path) {
        // Add path to array, this is expected as the last array element.
        $args[] = $path;
        return $rejsonClient->mget(...$args);
    }

@mkorkmaz
Copy link
Owner

Hi again @Peace-N

Command follows raw redis rejson command api. See details at https://oss.redislabs.com/redisjson/commands/#jsonmget

But you are right about one may want to provide keys as an array. We must allow that without breaking backward compatibility and rejson api compatibility.

Would you change this part in your code:

         $keys = array_shift($arguments); 

to this and send PR again?

        if (is_array($keys[0])) {
            $keys = $keys[0];
        }

And one more addition: Would you add a test case to cover this change. You can find the related test at:

public function shouldRunReJSONCommandMgetSuccessfully() : void

@mkorkmaz mkorkmaz closed this Apr 14, 2019
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

Successfully merging this pull request may close these issues.

None yet

2 participants