Skip to content

Prevent TypeError when using built-in constants in setOption. Fixes #1538#1540

Merged
michael-grunder merged 1 commit into
phpredis:developfrom
JoyceBabu:develop
Apr 4, 2019
Merged

Prevent TypeError when using built-in constants in setOption. Fixes #1538#1540
michael-grunder merged 1 commit into
phpredis:developfrom
JoyceBabu:develop

Conversation

@JoyceBabu

Copy link
Copy Markdown
Contributor

In strict_type mode Redis::setOption requires the second argument
to be of type string. This throws a TypeError for some options,
even when using built-in constants

The type of value argument of setOption has been changed to
mixed to prevent the error.

@JoyceBabu JoyceBabu changed the title Fix TypeError when using built-in constants in setOption Prevent TypeError when using built-in constants in setOption. Fixes 31538 Apr 4, 2019
Comment thread redis_commands.c Outdated
In strict_type mode `Redis::setOption` requires the second argument
to be of type `string`. This throws a TypeError for some options,
even when using built-in constants

The type of `value` argument of setOption has been changed to
mixed to prevent the error.
Comment thread redis_commands.c
if (val_str && val_len > 0) {
redis_sock->prefix = zend_string_init(val_str, val_len, 0);
val_str = zval_get_string(val);
if (ZSTR_LEN(val_str) > 0) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrchamp Do we need this check here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - I'm not as familiar with the zend code.

@JoyceBabu JoyceBabu changed the title Prevent TypeError when using built-in constants in setOption. Fixes 31538 Prevent TypeError when using built-in constants in setOption. Fixes #1538 Apr 4, 2019
@michael-grunder michael-grunder merged commit 4c7643e into phpredis:develop Apr 4, 2019
@michael-grunder

Copy link
Copy Markdown
Member

Merged, thanks!

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