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

[SITE-2632] Update check_client_dependencies to check for Relay #471

Closed
wants to merge 3 commits into from

Conversation

EarthlingDavey
Copy link
Contributor

This PR fixes 2 cases where WP_REDIS_USE_RELAY=true

  • The class is not checked for existence.
  • The drop-in will not run if Redis extension is not installed.

The fix:

  • Relay\Relay is correctly checked for existence.
  • The drop-in still works if the Redis extension is not installed.

This PR fixes 2 cases where `WP_REDIS_USE_RELAY=true`

- The class is not checked for existence.
- The drop-in will not run if Redis extension is not installed.

The fix:

- Relay\Relay is correctly checked for existence.
- The drop-in still works if the Redis extension is not installed.
@EarthlingDavey EarthlingDavey requested a review from a team as a code owner October 16, 2024 10:20
object-cache.php Outdated Show resolved Hide resolved
jazzsequence

This comment was marked as outdated.

Copy link
Contributor

@jazzsequence jazzsequence left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if WP_REDIS_USE_RELAY constant does not exist, PHP errors are thrown. More logic needed to handle if that constant exists before using it in the condition.

object-cache.php Outdated
@@ -1226,7 +1226,7 @@ protected function _connect_redis() {
* not with a message describing the issue.
*/
public function check_client_dependencies() {
if ( ! class_exists( 'Redis' ) ) {
if ( ! class_exists( WP_REDIS_USE_RELAY ? 'Relay\Relay' : 'Redis' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ( ! class_exists( WP_REDIS_USE_RELAY ? 'Relay\Relay' : 'Redis' ) ) {
$has_relay = defined( 'WP_REDIS_USE_RELAY' ) && class_exists( 'Relay\Relay' );
if ( ! $has_relay || ! class_exists( 'Redis' ) ) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah,I applied this change, but in fact we need to check defined('WP_REDIS_USE_RELAY') && WP_REDIS_USE_RELAY && class_exists( 'Relay\Relay' ), in the event the constant is set and false.

@jazzsequence
Copy link
Contributor

@pwtyler My suggested change should handle the condition that's failing the unit tests. Once merged, I think this is probably g2g. Needs changelog update and version bump if we're pushing a release.

@EarthlingDavey
Copy link
Contributor Author

Hi @jazzsequence , thanks for catching this and for the suggestion.

I can pick it up next week.

@pwtyler pwtyler changed the title Update check_client_dependencies to check for Relay [SITE-2632] Update check_client_dependencies to check for Relay Oct 30, 2024
Co-authored-by: Chris Reynolds <[email protected]>
@EarthlingDavey
Copy link
Contributor Author

Hey folks, apologies for not thoroughly testing my first PR. Having read all feedback, would the following be ok?

if ( ! class_exists( defined('WP_REDIS_USE_RELAY') && WP_REDIS_USE_RELAY ? 'Relay\Relay' : 'Redis' ) ) {

@jazzsequence
Copy link
Contributor

@EarthlingDavey at a glance, that looks like it would work. @pwtyler do you agree?

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