WIP: Update stubs so the tests pass in strict mode

[michael-grunder]

These changes allow the PHP 8 unit tests to pass even when zpp strict
mode is enabled.

I'm not exactly sure which setting causes the issue, but without these
changes I get many zpp errors if I run the tests inside of a PHP build
tree.

Fixes #2051, #2096, #1921

[michael-grunder]

@yatsukhnenko I will try to figure out what the actual setting is that causes the issues. Maybe it's if you're in a debug build...

[kkmuffme]

This PR could also fix #1921, as the stubs are correct now.
@michael-grunder could you apply those changes you did for the stubs also for the documentation, so it's correct there too?

[michael-grunder]

@kkmuffme I can do that, although I'm not sure whether the modifications constitute breaking changes, meaning we can't release until >= PhpRedis 6.0.

[kkmuffme]

That's fine, the docs have been wrong all this time.
Basically the docs are just missing that a lot of functions may return false if Redis is offline/loading data into memory,... (which you you have corrected in the stubs now, as that was wrong there too)

[orklah]

Hey @michael-grunder , I'm one of the maintainers of Psalm. We're struggling a lot recently with the definitions of phpredis methods to have a correct idea of what the current signature are (see vimeo/psalm#7709 (comment))

Would you mind giving us a quick summary of where we could find an authoritative source for what's the current stable version of phpredis and how it's planned to evolve?

We had old definitions until recently when we tried to use your stub here as source, but it seems to mismatch the current stable version. We're not sure if this PR is supposed to be a future version, a WIP or what.

Please tell me if you prefer me to open an issue or a discussion about this, I'll be glad to :)

Thanks!

[michael-grunder]

Would you mind giving us a quick summary of where we could find an authoritative source for what's the current stable version of phpredis and how it's planned to evolve?

The most accurate prototype information is in the .stub files from this branch, which should be completely correct. I'll need to rebase against develop to get it merged but the changes shouldn't be large.

@yatsukhnenko Are you opposed to getting this merged for release in >= 6.0.0?

[orklah]

The most accurate prototype information is in the .stub files from this branch, which should be completely correct

Thanks! That's good to know!

We have a feedback from a user who seem to show the setOption should expect an int, not a string, contrary to the stub here, maybe something to fix?

Also, do you intend to add constants to this stub? We tend to add them in stubs on Psalm but this may not be the custom here

[yatsukhnenko]

@michael-grunder go ahead 🙂

[michael-grunder]

We have a feedback from a user who seem to show the setOption should expect an int, not a string, contrary to the stub here, maybe something to fix?

Good catch, thanks. I'll update the branch.

Also, do you intend to add constants to this stub? We tend to add them in stubs on Psalm but this may not be the custom here

The last time I checked, the zpp generation logic in PHP didn't yet support class constants, but I'll see if that's since changed.

[orklah]

We have another user who is surprised to see the Redis instance returned by some methods: vimeo/psalm#7709 (comment) (for example on exists). Is this expected behaviour? Maybe on some special conditions?

[michael-grunder]

We have another user who is surprised to see the Redis instance

Yes, that's one of the main fixes introduced in the branch. Nearly all methods can return the Redis object if the user is within a PIPELINE or MULTI block.

e.g.

$redis->pipeline();

// These commands will both return the Redis object
$redis->set('foo', 'bar');
$redis->incr('counter');

// Returns an array with the result of the above two commands
$result = $redis->exec(); 

[orklah]

Oh thanks! That might need a special handling in Psalm if we want to be super precise!

[ADmad]

Yes, that's one of the main fixes introduced in the branch. Nearly all methods can return the Redis object if the user is within a PIPELINE or MULTI block.

Dealing with this is going to be a PITA 🙁. Lot of user code assumes the methods like Redis::set()/setEx()return bool only. With this stub added to psalm we now have errors at multiple places where Redis class is used.

[michael-grunder]

Dealing with this is going to be a PITA

Agreed, it's breaking as hell which is why we can't officially release it until PhpRedis >= 6.0.0.

Is there any way we can lessen the pain from this end?

[kkmuffme]

Also the constants are missing in the stubs and the docs (@todo in https://github.com/phpredis/phpredis#predefined-constants)

I just grepped them out of the code base: Redis::[A-Z_]+

Redis::AFTER
Redis::BACKOFF_ALGORITHM_CONSTANT
Redis::BACKOFF_ALGORITHM_DECORRELATED_JITTER
Redis::BACKOFF_ALGORITHM_DEFAULT
Redis::BACKOFF_ALGORITHM_EQUAL_JITTER
Redis::BACKOFF_ALGORITHM_EXPONENTIAL
Redis::BACKOFF_ALGORITHM_FULL_JITTER
Redis::BACKOFF_ALGORITHM_UNIFORM
Redis::BEFORE
Redis::COMPRESSION_LZ
Redis::COMPRESSION_LZF
Redis::COMPRESSION_NONE
Redis::COMPRESSION_ZSTD
Redis::LEFT
Redis::MULTI
Redis::OPT_BACKOFF_ALGORITHM
Redis::OPT_BACKOFF_BASE
Redis::OPT_BACKOFF_CAP
Redis::OPT_COMPRESSION
Redis::OPT_COMPRESSION_LEVEL
Redis::OPT_MAX_RETRIES
Redis::OPT_NULL_MULTIBULK_AS_NULL
Redis::OPT_PREFIX
Redis::OPT_READ_TIMEOUT
Redis::OPT_REPLY_LITERAL
Redis::OPT_SCAN
Redis::OPT_SERIALIZER
Redis::PIPELINE
Redis::REDIS_HASH
Redis::REDIS_LIST
Redis::REDIS_NOT_FOUND
Redis::REDIS_SET
Redis::REDIS_STREAM
Redis::REDIS_STRING
Redis::REDIS_ZSET
Redis::RIGHT
Redis::SCAN_NOPREFIX
Redis::SCAN_NORETRY
Redis::SCAN_PREFIX
Redis::SCAN_RETRY
Redis::SERIALIZER_IGBINARY
Redis::SERIALIZER_JSON
Redis::SERIALIZER_MSGPACK
Redis::SERIALIZER_NONE
Redis::SERIALIZER_PHP

[kkmuffme]

btw the new stubs are amazing, not a PITA at all. In fact I could remove so many static analysis suppressions from the code base and fix a few issues that didn't come up with the last stubs.

[kkmuffme]

I think it should be changed, to only accept string types for set functions (set, mset,...)
While technically it is possible to pass mixed (bool, int,...) you will ALWAYS get back a string (as correctly defined for function get) - which may not be expected and lead to bugs that need to be tracked down.

[kkmuffme]

Also this issue should be taken care of #2120 (ideally in the docs too?)

[michael-grunder]

@yatsukhnenko I think maybe we merge it in preparation for PhpRedis 6.0.0?

We should also both go over the codebase and find any other things we'd like to break 😈

[yatsukhnenko]

@michael-grunder lets merge it