endclothing / prometheus_client_php

Prometheus instrumentation library for PHP applications
https://prometheus.io/docs/concepts/metric_types/
Apache License 2.0
150 stars 77 forks source link

Type error in Redis usage #34

Open MyIgel opened 4 years ago

MyIgel commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

The first issue has to be fixed by casting it to a string to be phpredis < 5 compatible, see https://github.com/phpredis/phpredis/issues/1538

SteenSchutt commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.. 63966cdd9402a08a22e184038420b358b14e1dbe #20

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

fernandesGabriel commented 4 years ago

@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 commented 4 years ago

Sorry @fernandesGabriel - I no longer work at EndClothing.