Skip to content

Conversation

ondrejmirtes
Copy link
Member

No description provided.

@RobiNN1
Copy link

RobiNN1 commented Jun 13, 2025

I closed my PR, because it will not be merged anyway. Now I'm checking that reflection test.

But where is this from? (There are many issues like this)

     /**
      * @param string $key
-     * @param int $min
-     * @param int $max
-     * @param int $options
-     * @param int $limit
-     * @return array
+     * @param string $min
+     * @param string $max
+     * @param array|null $options
+     * @return array|bool|RedisCluster
      */
-    function zRevRangeByLex(string $key, string $min, string $max, array|null $options = null, mixed $limit): array|bool|RedisCluster'
+    function zRevRangeByLex(string $key, string $min, string $max, array|null $options = null): array|bool|RedisCluster'

Here is from phpstorm stub file which was recently updated JetBrains/phpstorm-stubs#1750

    /**
     * @param   string $key
     * @param   string $min
     * @param   string $max
     * @param   null|array $options
     *
     * @return  RedisCluster|bool|array
     * @throws  RedisClusterException
     * @see     zRangeByLex()
     *
     * @link    https://redis.io/commands/zrevrangebylex
     */
    public function zRevRangeByLex(string $key, string $min, string $max, ?array $options = null): RedisCluster|bool|array {}

There are few phpdocs issues but function signatures are correct.

@ondrejmirtes
Copy link
Member Author

Yeah, that's expected. The new (+ lines in the diff) match exactly what's in phpstorm-stubs. Because I removed the old (wrong) signatures from functionMap.php.

@VincentLanglet
Copy link
Contributor

I don't think it will be possible to rely on phpstorm-stubs for redis because we're using __benevolent types for some return values. This PR might be closed without replacement, and we'll fix redis issues one by one when reported...

@RobiNN1
Copy link

RobiNN1 commented Oct 5, 2025

There are way too much changes since RedisCluster stubs are outdated here. I sent PR for this before, but it was hard to review.

Anyway I noticed these issues with latest phpstan

Method RedisCluster::rawcommand() invoked with 2 parameters, 3 required

public function rawcommand(string|array $key_or_address, string $command, mixed ...$args): mixed {} $args is optional

Method RedisCluster::info() invoked with 2 parameters, 0-1 required.
public function info(string|array $key_or_address, string ...$sections): RedisCluster|array|false {}

In most of signatures is missing new string|array $key_or_address

@VincentLanglet
Copy link
Contributor

VincentLanglet commented Oct 5, 2025

There are way too much changes since RedisCluster stubs are outdated here. I sent PR for this before, but it was hard to review.

I recommand to reopen https://github.com/phpstan/phpstan-src/pull/4045/files but with smaller PR, you could

  • Fix existing definition in one PR
  • Add new definition in another PR

And if the fix existing definition is still to big you can split into smaller BR, maybe

  • One PR to add missing parameters
  • One PR to fix some param type
  • One PR to fix return type

Or limiting PR at 5-10 functions signatures fixes

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