fix: Fixes assumption that CACHE_PORT & CACHE_PASSWORD are Set.

[timnolte]

• Fixes #359
• Falls back on port 6379 if the CACHE_PORT is not configured.
• Doesn't require a CACHE_PASSWORD to be set when it isn't used, or can't be.
• Improves code quality by reducing Redis default port & databasei duplicate values to a share variables.
• Fixes invalid code changes made in PR #400 to the core plugin connectivity testing that prevent connectivty checks if the port/password/database aren't explicitly defined.

[timnolte]

@danielbachhuber it is not clear to me why these tests have failed. I'm not seeing which tests I may need to update to account for this change.

[strarsis]

@timnolte: Thanks for this PR! So it seems that this error occurs when the memory limit of the CI test system is exhausted: https://swiftmade.co/blog/2022-04-05-phpunit-failing-with-exit-code-255/ Can the memory limit be boosted for the CI test system in your fork/PR and does this fix the failing test (that fails due to other reasons)?

[timnolte]

@strarsis so I attempted to add an option to increase the memory during testing but I don't think CircleCI will run the configuration changes in my branch. I'm also seeing an authentication error in the CircleCI build output.

[strarsis]

@timnolte: The modifications for increasing the memory limit seem to cause issues, some shell thing:

We trust you have received the usual lecture from the local System
Administrator. It usually boils down to these three things:

    #1) Respect the privacy of others.
    #2) Think before you type.
    #3) With great power comes great responsibility.

[sudo] password for tester: 
sudo: no password was provided
sudo: a password is required

Exited with code exit status 1
CircleCI received exit code 1

sudo is used, but the runner doesn't expect a sudo password prompt, hence the task fails.

[timnolte]

@timnolte: The modifications for increasing the memory limit seem to cause issues, some shell thing:

We trust you have received the usual lecture from the local System
Administrator. It usually boils down to these three things:

    #1) Respect the privacy of others.
    #2) Think before you type.
    #3) With great power comes great responsibility.

[sudo] password for tester: 
sudo: no password was provided
sudo: a password is required

Exited with code exit status 1
CircleCI received exit code 1

sudo is used, but the runner doesn't expect a sudo password prompt, hence the task fails.

Well, given that the 255 error was already happening I didn't see that. I took the route of might need to use sudo just in case as documented by the CircleCI Support. https://support.circleci.com/hc/en-us/articles/360040700954-Increasing-PHP-s-memory-limit

I can try it without sudo.

[strarsis]

@timnolte: This CI stuff is very annoying as testing it locally is not always possible or easily possible. Hopefully the tests can be made running again so this PR can be merged.

[strarsis]

1. So sudo is needed to write into the PHP configuration for increasing the memory limit.
2. sudo doesn't work with this runner/CircleCI image. There is a warning about using an outdated Docker convenience image for these tests: https://discuss.circleci.com/t/legacy-convenience-image-deprecation/41034/1 🤔

This approach changes the memory limit directly for PHP unit using the PHP unit CLI instead, this may be easier: https://discuss.circleci.com/t/php-memory-size/7602/2

[timnolte]

@strarsis I tried without sudo however I'm seeing authentication errors for behat like

#!/bin/bash -eo pipefail
./bin/behat-prepare.sh

 [notice] You are not logged in.
+ terminus env:create wp-redis.dev ci-2974

In Authorizer.php line 37:

  You are not logged in. Run `auth:login` to authenticate or `help auth:login  
  ` for more info.

[strarsis]

@timnolte: This approach changes the memory limit directly for PHP unit using the PHP unit CLI instead, this may be easier: https://discuss.circleci.com/t/php-memory-size/7602/2

phpunit -d memory_limit=512M

So 1GB or more can ensure there is enough memory during the test run: phpunit -d memory_limit=1G

[strarsis]

Now there are other errors (a good thing?):

In Authorizer.php line 37:

  You are not logged in. Run `auth:login` to authenticate or `help auth:login  
  ` for more info.                                                             

Shouldn't this be already handled by the test setup?

Err:4 https://dl.google.com/linux/chrome/deb stable InRelease
  The following signatures couldn't be verified because the public key is not available: NO_PUBKEY 4EB27DB2A3B88B8B
Reading package lists... Done
N: Repository 'http://deb.debian.org/debian bullseye InRelease' changed its 'Version' value from '11.1' to '11.4'
W: GPG error: https://dl.google.com/linux/chrome/deb stable InRelease: The following signatures couldn't be verified because the public key is not available: NO_PUBKEY 4EB27DB2A3B88B8B
E: The repository 'https://dl.google.com/linux/chrome/deb stable InRelease' is not signed.

Those apt errors often resolve on their own. 😞

[timnolte]

@strarsis I saw those same errors and I have no idea what to do about them. The authorization issue is for terminus and I'm pretty sure it's because there is no access to the keys within CircleCI for them to run successfully for my branch. GitHub Actions and other CI/CD platforms have the same problems.

[timnolte]

@greg-1-anderson @danielbachhuber @kporras07 just trying to get some eyes on this in light of PHP 8 compatibility and this causing a bunch of warnings/errors when no password is used.

[timnolte]

@greg-1-anderson @danielbachhuber @kporras07 just trying to get some eyes on this in light of PHP 8 compatibility and this causing a bunch of warnings/errors when no password is used.

Just pinging this again.

[jazzsequence]

@timnolte We have an open issue that we are tracking internally (CMS-1119) for PHP 8.x support for the wp-redis plugin. The issue has been story pointed and is ready to bring into a sprint is not part of our current sprint. I have added this PR to our internal ticket to ensure that this gets eyes on it when we are working that ticket.

[jspellman814]

@timnolte Thanks for your contribution. FWIW we've fixed the tests and included this in the 1.3.0 release. If you were to merge master into your branch and push up, I'd expect the tests to pass.

We've also made some adjustments to our workflow to better work with automated deployments to wp.org. See CONTRIBUTING.md. Could you make this PR against pantheon-systems:develop? Thank you!

[timnolte]

I hadn't had a chance to get back to this but I will try to carve out some time to update my branch so that this can be officially fixed. We've been patching the plugin via composer for awhile now.

[strarsis]

@timnolte: Awesome, thank you!

[timnolte]

Noting that there were changes made that started to fix this in https://github.com/pantheon-systems/wp-redis/pull/400 however those changes were wrong and it's still broke.

[jazzsequence]

@timnolte can you rebase or merge in the current default branch to resolve the merge conflicts since this PR is out-of-date now?

[timnolte]

@jazzsequence I've completely redone this work. I've already recreated my patch that I'm pushing out to our own projects via Composer patching.

[jazzsequence]

@timnolte thanks for updating the PR. I will use this in favor of #428.

[timnolte]

For those that need an immediate patch, that can be applied via Composer, for these changes you can get it here: https://gist.github.com/timnolte/267e2a502f05156c0fe2a0d7028c2854

[jazzsequence]

actually, going to port these changes into #428 so automated testing can still run (since those don't work on PRs from outside contributors).

[timnolte]

actually, going to port these changes into #428 so automated testing can still run (since those don't work on PRs from outside contributors).

Hmm, that's rather disappointing that outside contributors won't get any credit for their work. :-(

[jazzsequence]

Yeah, not sure what the issue is exactly, but we'll add you to the release props.

[jazzsequence]

you know what? if it's passing in #428 then I'm gonna just close that and use this instead. Just wanna pull in my changelog update and then will approve & merge.

[timnolte]

@jazzsequence FYI, so something that is interesting is that I'm finding now that this change somehow doesn't work on Pantheon yet is working on non-Pantheon Redis environments. I'm working on testing this out further to see what the issue is on Pantheon vs non-Pantheon platforms.

[rwagner00]

@timnolte To clarify, do you mean that it's not resolving the original issue, or that it actively causes new issues in Pantheon environments?

[timnolte]

It appears to cause problems in Pantheon environments. I'm working on a new patch as I think the problem is with the changes that we're originally made to use isset() instead of ! empty() checks. My original patch that had been applied to our sites, including Pantheon, didn't have this problem but was also using ! empty().

[timnolte]

So, I've tried another patch and this just seems strange. Something more changed in 1.4.2 that breaks connectivity on Pantheon with this fix, whereas prior to all of these changes in 1.4.2(at least as far back as 1.3.4) this was not a problem. With the current patches I'm now getting connection refused errors with Redis.

Warning: WP Redis: Connection refused in /code/web/wp-content/plugins/wp-redis/object-cache.php on line 1478

[timnolte]

So, this is interesting as I thought that Pantheon was using Redis server v6 along with the PHP 8.0 hosting, and Redis server v5 along with PHP 7.4. However, I'm connecting to the remote instances and everything is telling me that Pantheon is running some super old v2.8 Redis servers. 😱

[rwagner00]

@timnolte While we don't have anything public to announce right at this moment, someone from Pantheon will be reaching out to you in Slack regarding Redis versions.

[timnolte]

@jazzsequence so I believe I found the issue in the current version of the plugin. This line(https://github.com/pantheon-systems/wp-redis/blob/default/object-cache.php#L1278) is wrong as the function attributes are backwards.

https://www.php.net/manual/en/function.array-replace-recursive.php

replaces the values of array with the same values from all the following arrays

[timnolte]

For those that need an immediate patch, that can be applied via Composer, for these changes you can get it here: https://gist.github.com/timnolte/267e2a502f05156c0fe2a0d7028c2854

I have updated this patch with the correct fixes so that some of the changes in this PR actually work on Pantheon hosting.

[timnolte]

@jazzsequence just a note that I have opened up an issue regarding the array_replace_recursive() issue and a PR to fix it. https://github.com/pantheon-systems/wp-redis/pull/434