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

Type error in Redis usage #34

Open
MyIgel opened this issue Dec 18, 2019 · 7 comments
Open

Type error in Redis usage #34

MyIgel opened this issue Dec 18, 2019 · 7 comments

Comments

@MyIgel
Copy link

MyIgel commented Dec 18, 2019

Running

<?php

declare(strict_types=1);

$redis = new Redis();
$redis->connect('127.0.0.1', 6379, 0.1);
$redis->setOption(Redis::OPT_READ_TIMEOUT, 10);

leads to a TypeError error in PHP 7.3.10:

$ docker run -it --rm alpine:latest
/ # apk add php7 php7-redis
fetch http://dl-cdn.alpinelinux.org/alpine/v3.10/main/x86_64/APKINDEX.tar.gz
fetch http://dl-cdn.alpinelinux.org/alpine/v3.10/community/x86_64/APKINDEX.tar.gz
(1/12) Installing php7-common (7.3.11-r0)
(2/12) Installing argon2-libs (20171227-r2)
(3/12) Installing ncurses-terminfo-base (6.1_p20190518-r0)
(4/12) Installing ncurses-terminfo (6.1_p20190518-r0)
(5/12) Installing ncurses-libs (6.1_p20190518-r0)
(6/12) Installing libedit (20190324.3.1-r0)
(7/12) Installing pcre2 (10.33-r0)
(8/12) Installing libxml2 (2.9.9-r2)
(9/12) Installing php7 (7.3.11-r0)
(10/12) Installing php7-pecl-igbinary (3.0.1-r1)
(11/12) Installing php7-session (7.3.11-r0)
(12/12) Installing php7-pecl-redis (4.3.0-r2)
Executing busybox-1.30.1-r2.trigger
OK: 20 MiB in 26 packages
/ # echo '[...]' > redistest.php
/ # php redistest.php; echo $?
PHP Fatal error:  Uncaught TypeError: Redis::setOption() expects parameter 2 to be string, int given in /redistest.php:7
Stack trace:
#0 /redistest.php(7): Redis->setOption(3, 10)
#1 {main}
  thrown in /redistest.php on line 7
255

Using a alpine:latest container with the php7 php7-redis packages (PHP 7.3.10 (cli) (built: Oct 3 2019 11:21:47) ( NTS ))

To me that looks more like it could be a bug in the redis package but sadly i have no deep enough knowledge to investigate that further.
It got fixed in phpredis >=5 but that makes this plugin incompatible to phpredis 4 which is currently distributed.

@MyIgel MyIgel changed the title Type error in Redis implementation Type error in Redis usage Dec 18, 2019
@MyIgel
Copy link
Author

MyIgel commented Dec 18, 2019

Another error that ocured is that, when using docker, the port parameter is set as a string but Redis::connect() requires an int here (a simple type cast before the env(...) or better in the connect(...) call should fix that).

Redis::connect() expects parameter 2 to be int, string given in /vendor/endclothing/prometheus_client_php/src/Prometheus/Storage/Redis.php:132

@MyIgel
Copy link
Author

MyIgel commented Dec 18, 2019

The first issue has to be fixed by casting it to a string to be phpredis < 5 compatible, see phpredis/phpredis#1538

@SteenSchutt
Copy link

SteenSchutt commented Dec 30, 2019

I am experiencing the same error on PHP 7.3.10 and 7.3.13 (With php-pecl-redis4 from the remi repository on CentOS 7). Downgraded the package back to 0.9.1 while waiting for a fix.

@SteenSchutt
Copy link

I can now confirm that replacing php-pecl-redis (which is an alias for php-pecl-redis4) with php-pecl-redis5 on CentOS 7 will resolve the issue.

I also noticed, while upgrading this library, that the version key is gone from composer.json as of 1.0.2, which is probably why that version is not on Packagist (where the latest available is 1.0.1).

@NoelDavies
Copy link

NoelDavies commented Jan 6, 2020

I can now confirm that replacing php-pecl-redis (which is an alias for php-pecl-redis4) with php-pecl-redis5 on CentOS 7 will resolve the issue.

I also noticed, while upgrading this library, that the version key is gone from composer.json as of 1.0.2, which is probably why that version is not on Packagist (where the latest available is 1.0.1).

I've just noticed it was changed during CI builds, not sure why.. 63966cd #20

I completely missed that, would be good to have others reviewing these PRs too :P haha

@fernandesGabriel
Copy link

fernandesGabriel commented May 4, 2020

@NoelDavies do you intend to keep distributing these Dockerfiles with the project? I had similar problem as the guys there, and I can tidy a little the setup and make a PR if you plan to keep those dev containers. I personally would just remove from this project and maybe create another as helper to test this one, or even just make available the Docker images and the pure docker command to execute the tests while contributing.

@NoelDavies
Copy link

Sorry @fernandesGabriel - I no longer work at EndClothing.

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

4 participants