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 compatibility with \Redis::script() #74

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

her-ur
Copy link

@her-ur her-ur commented Sep 19, 2017

No description provided.

@enumag
Copy link
Member

enumag commented Sep 19, 2017

According to the documentation the second argument is optional. https://github.com/phpredis/phpredis#script

@her-ur
Copy link
Author

her-ur commented Sep 19, 2017

Yes, you're right. I actually getting this warning on Debian with php5-redis 2.2.5-1:
Declaration of Kdyby\Redis\Driver\PhpRedisDriver::script($command, $script = NULL) should be compatible with Redis::script($cmd, ...$args)
Any thoughts what should i do with that? Thank you very much!

@enumag
Copy link
Member

enumag commented Sep 19, 2017

Oh it seems they updated the API with PHP Argument Unpacking (https://wiki.php.net/rfc/argument_unpacking). Which means you should change the function to something like this:

public function script($command, ...$args)
{
    return parent::script($command, ...$args);
}

Of course you would have to raise the required PHP version in composer.json to 5.6.

However this seems completely useless. The question here is why Kdyby overrides these methods in the first place since it would be equivalent if the methods were not there. Unfortunately I do not know this.

@enumag
Copy link
Member

enumag commented Sep 19, 2017

What happens if you simply remove the script method from Kdyby\Redis\Driver\PhpRedisDriver?

@her-ur
Copy link
Author

her-ur commented Sep 20, 2017

It is solution for me, but this method is defined also in IRedisDriver and that interface is used in Nette DI extension. So it would be BC break.

@jelen07
Copy link

jelen07 commented Nov 30, 2017

@her-ur Which DI extension use this interface?

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.

3 participants