danog / MadelineProto

Async PHP client API for the telegram MTProto protocol
https://docs.madelineproto.xyz
GNU Affero General Public License v3.0
2.74k stars 617 forks source link

Redis Settings overrides database number and password when URI contains these values #1523

Closed xdimedrolx closed 1 month ago

xdimedrolx commented 1 month ago

There is an issue in the getOrmSettings method of the Redis class where if the URI contains a database number or password, these values are overridden by the default values set in the class properties

https://github.com/danog/MadelineProto/blob/79d58d6991636a9eb3767a8ae2c3c386e3ab8853/src/Settings/Database/Redis.php#L33

danog commented 1 month ago

Make sure you provide the redis settings every time you pass a settings object to MadelineProto

xdimedrolx commented 1 month ago

I initialize it like this:

$redis = new danog\MadelineProto\Settings\Database\Redis();
$redis->setUri('redis://:secret@localhost:6379/2');

$settings = new \danog\MadelineProto\Settings();
$settings->setDb($redis);

$madelineProto = new \danog\MadelineProto\API('session.22986706', $settings);

I expect that the password "secret" and database 2 will be used. However, the connection occurs without a password and to database 1.

To fix this, I did the following:

public function getOrmSettings(): Settings
{
    $config = RedisConfig::fromUri($this->uri);
    if ($this->database !== 0) {
        $config->withDatabase($this->database);
    }
    if ($this->password !== '') {
        $config->withPassword($this->password);
    }

    return new RedisSettings(
        $config,
        match ($this->serializer) {
            SerializerType::IGBINARY => new Igbinary,
            SerializerType::SERIALIZE => new Native,
            null => null
        },
        $this->cacheTtl,
    );
}

I can create a pull request if this solution is correct.

danog commented 1 month ago

This is an issue with the amphp/redis repo, please reopen the issue/PR there.