pantheon-systems / wp-redis

WordPress Object Cache using Redis.
https://wordpress.org/plugins/wp-redis/
GNU General Public License v2.0
225 stars 67 forks source link

[CMSP-459] bugfix: re-add missing else #437

Closed jazzsequence closed 1 year ago

jazzsequence commented 1 year ago

This re-adds the else statement that was inadvertently missed in a recent commit, leading to some of the Behat test failures.

This, as well as #430 account for all of the environment fixes that were mixed into #426 but separate from the actual change suggested in that PR.

Tests can be seen as passing (with #430) here: https://app.circleci.com/pipelines/github/pantheon-systems/wp-redis/1629/workflows/fd714abf-075f-45f8-9102-3ddc730f731e/jobs/4980

pwtyler commented 1 year ago

that was inadvertently missed in a recent commit

Can you link? Was this #428?

jazzsequence commented 1 year ago

@pwtyler https://github.com/pantheon-systems/wp-redis/commit/69c194fe4f3929eaafad5baae0e8b374ebb3771f

        } else { // tcp connection.
            $port = ! empty( $redis_server['port'] ) ? $redis_server['port'] : 6379;
        }

We can make improvements, but this is just to get us back to the baseline working. Any additional changes should (IMO) be in a separate PR.

And you can see in #430 that without this change, the tests are failing.

pwtyler commented 1 year ago

We can make improvements, but this is just to get us back to the baseline working.

+1 on the simpler fix first and ignore my second comment, ternary operator w/ fallback is still unnecessary if $port has a default value at the top of the func

jazzsequence commented 1 year ago

@pwtyler There is a chance that the $redis_server value passed into the function might be empty -- e.g.

$redis_server = [
    'host' => '127.0.0.1',
    'database' => 0,
];
$params = build_client_parameters( $redis_server );

In that case, $redis_server['port'] would never be set and the final return value would be missing a port because there would be no port in either $redis_server or $defaults. I think the ternary is still required.

timnolte commented 1 year ago

@jazzsequence & @pwtyler just noting that this change was actually incorrectly made as the real issue was the misused of array_replace_recursive(). I have fixed this again in my PR. https://github.com/pantheon-systems/wp-redis/pull/434