Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jun 23, 2023

Conversation

jazzsequence
Copy link
Contributor

@jazzsequence jazzsequence commented Jun 22, 2023

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

@jazzsequence jazzsequence requested review from a team as code owners June 22, 2023 21:16
@jazzsequence jazzsequence changed the title bugfix: re-add missing else [CMSP-459] bugfix: re-add missing else Jun 22, 2023
@pwtyler
Copy link
Member

pwtyler commented Jun 22, 2023

that was inadvertently missed in a recent commit

Can you link? Was this #428?

object-cache.php Outdated Show resolved Hide resolved
@jazzsequence
Copy link
Contributor Author

jazzsequence commented Jun 22, 2023

@pwtyler 69c194f

		} 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
Copy link
Member

pwtyler commented Jun 22, 2023

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
Copy link
Contributor Author

jazzsequence commented Jun 23, 2023

@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.

@jazzsequence jazzsequence merged commit d1b6103 into default Jun 23, 2023
6 checks passed
@jazzsequence jazzsequence deleted the cmsp-459-add-missing-else branch June 23, 2023 16:59
timnolte added a commit to timnolte/wp-redis that referenced this pull request Jun 23, 2023
* Reverts this incorrect change that was made due to the incorrect use of `array_replace_recursive()`.
@timnolte
Copy link
Contributor

@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. #434

jazzsequence pushed a commit to timnolte/wp-redis that referenced this pull request Jun 23, 2023
* Reverts this incorrect change that was made due to the incorrect use of `array_replace_recursive()`.
jazzsequence added a commit that referenced this pull request Jun 23, 2023
…er issues (#434)

* fix: Fixes incorrect order of array_replace_recursive arguments & other issues

* Fixes #433
* Fixes #432
* Fixes #431
* Further clean-up & standardization between object-cache.php & wp-redis.php.
* Fixes incorrect order of array_replace_recursive arguments.
* Addresses issue with port still not being null for socket connections due to defaults array_repalce_recursive use.

* fix: Fixes sanitization methods and linting issues

* Adjusts some items to use type-based sanitization.
* Adds linting expection handling with comments for cases that require it.

* fix: Removes invalid change made in #437

* Reverts this incorrect change that was made due to the incorrect use of `array_replace_recursive()`.

* update  changelog

* Update wp-redis.php

* update language in changelogs

* fix missing closing )

---------

Co-authored-by: Chris Reynolds <chris@jazzsequence.com>
Co-authored-by: Phil Tyler <philip@tylerdigital.com>
jazzsequence added a commit that referenced this pull request Jun 26, 2023
* re-add missing else

* use the already-defined value of $port instead of hard-coding

* remove ternary in favor of elseif
jazzsequence added a commit that referenced this pull request Jun 26, 2023
…er issues (#434)

* fix: Fixes incorrect order of array_replace_recursive arguments & other issues

* Fixes #433
* Fixes #432
* Fixes #431
* Further clean-up & standardization between object-cache.php & wp-redis.php.
* Fixes incorrect order of array_replace_recursive arguments.
* Addresses issue with port still not being null for socket connections due to defaults array_repalce_recursive use.

* fix: Fixes sanitization methods and linting issues

* Adjusts some items to use type-based sanitization.
* Adds linting expection handling with comments for cases that require it.

* fix: Removes invalid change made in #437

* Reverts this incorrect change that was made due to the incorrect use of `array_replace_recursive()`.

* update  changelog

* Update wp-redis.php

* update language in changelogs

* fix missing closing )

---------

Co-authored-by: Chris Reynolds <chris@jazzsequence.com>
Co-authored-by: Phil Tyler <philip@tylerdigital.com>
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.

None yet

3 participants