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

Improve remote specifications transient handling and error management #44384

Merged
merged 13 commits into from
Feb 15, 2024
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: update

Improve remote specifications transient handling and error management
26 changes: 20 additions & 6 deletions plugins/woocommerce/src/Admin/DataSourcePoller.php
Original file line number Diff line number Diff line change
Expand Up @@ -133,12 +133,12 @@ public function read_specs_from_data_sources() {
$this->merge_specs( $specs_from_data_source, $specs, $url );
}

$specs_group = get_transient( $this->args['transient_name'] ) ?? array();
$specs_group = get_transient( $this->args['transient_name'] );
$specs_group = is_array( $specs_group ) ? $specs_group : array();
Copy link
Member Author

Choose a reason for hiding this comment

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

Fix PHP 8.1+ Automatic conversion of false to array is deprecated

$locale = get_user_locale();
$specs_group[ $locale ] = $specs;
// Persist the specs as a transient.
set_transient(
$this->args['transient_name'],
$this->set_specs_transient(
$specs_group,
$this->args['transient_expiry']
);
Expand All @@ -154,6 +154,20 @@ public function delete_specs_transient() {
return delete_transient( $this->args['transient_name'] );
}

/**
* Set the specs transient.
*
* @param array $specs The specs to set in the transient.
* @param int $expiration The expiration time for the transient.
*/
public function set_specs_transient( $specs, $expiration = 0 ) {
chihsuan marked this conversation as resolved.
Show resolved Hide resolved
set_transient(
$this->args['transient_name'],
$specs,
$expiration,
);
}

/**
* Read a single data source and return the read specs
*
Expand Down Expand Up @@ -183,7 +197,7 @@ protected static function read_data_source( $url ) {
// phpcs:ignore
$logger->error( print_r( $response, true ), $logger_context );

return [];
return array();
Copy link
Contributor

Choose a reason for hiding this comment

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

I couldn't quite figure out the intent of changing [] for array(), is it a lint issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's a lint issue.

Short array syntax is not allowedPHP_CodeSniffer(Generic.Arrays.DisallowShortArraySyntax.Found)

}

$body = $response['body'];
Expand All @@ -195,7 +209,7 @@ protected static function read_data_source( $url ) {
$logger_context
);

return [];
return array();
}

if ( ! is_array( $specs ) ) {
Expand All @@ -204,7 +218,7 @@ protected static function read_data_source( $url ) {
$logger_context
);

return [];
return array();
}

return $specs;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,32 @@ public static function evaluate( $spec ) {

return $suggestion;
}

chihsuan marked this conversation as resolved.
Show resolved Hide resolved

/**
* Evaluates the specs and returns the visible suggestions.
*
* @param array $specs payment suggestion spec array.
* @return array The visible suggestions and errors.
*/
public static function evaluate_specs( $specs ) {
$suggestions = array();
$errors = array();

foreach ( $specs as $spec ) {
try {
$suggestion = self::evaluate( $spec );
if ( ! property_exists( $suggestion, 'is_visible' ) || $suggestion->is_visible ) {
$suggestions[] = $suggestion;
}
} catch ( \Throwable $e ) {
$errors[] = $e;
}
}

return array(
'suggestions' => $suggestions,
'errors' => $errors,
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,30 +35,24 @@ public function __construct() {
* @return array
*/
public static function get_suggestions( array $specs = null ) {
$suggestions = array();
if ( null === $specs ) {
$specs = self::get_specs();
}
$locale = get_user_locale();

$specs = is_array( $specs ) ? $specs : self::get_specs();
$results = EvaluateSuggestion::evaluate_specs( $specs );

foreach ( $specs as $spec ) {
try {
$suggestion = EvaluateSuggestion::evaluate( $spec );
$suggestions[] = $suggestion;
// phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch
} catch ( \Throwable $e ) {
// Ignore errors.
}
// When suggestions is empty, replace it with defaults and save for 3 hours.
if ( empty( $results['suggestions'] ) ) {
PaymentGatewaySuggestionsDataSourcePoller::get_instance()->set_specs_transient( array( $locale => DefaultPaymentGateways::get_all() ), 3 * HOUR_IN_SECONDS );

return EvaluateSuggestion::evaluate_specs( DefaultPaymentGateways::get_all() )['suggestions'];
}

return array_values(
array_filter(
$suggestions,
function( $suggestion ) {
return ! property_exists( $suggestion, 'is_visible' ) || $suggestion->is_visible;
}
)
);
// When suggestions is not empty but has errors, save it for 3 hours.
if ( count( $results['errors'] ) > 0 ) {
PaymentGatewaySuggestionsDataSourcePoller::get_instance()->set_specs_transient( array( $locale => $specs ), 3 * HOUR_IN_SECONDS );
}

return $results['suggestions'];
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,44 @@ public static function evaluate( $extension ) {

return $extension;
}

/**
* Evaluates the specs and returns the bundles with visible extensions.
*
* @param array $specs extensions spec array.
* @param array $allowed_bundles Optional array of allowed bundles to be returned.
* @return array The bundles and errors.
*/
public static function evaluate_bundles( $specs, $allowed_bundles = array() ) {
$bundles = array();

foreach ( $specs as $spec ) {
$spec = (object) $spec;
$bundle = (array) $spec;
$bundle['plugins'] = array();

if ( ! empty( $allowed_bundles ) && ! in_array( $spec->key, $allowed_bundles, true ) ) {
continue;
}

$errors = array();
foreach ( $spec->plugins as $plugin ) {
try {
$extension = self::evaluate( (object) $plugin );
if ( ! property_exists( $extension, 'is_visible' ) || $extension->is_visible ) {
$bundle['plugins'][] = $extension;
}
} catch ( \Throwable $e ) {
$errors[] = $e;
}
}

$bundles[] = $bundle;
}

return array(
'bundles' => $bundles,
'errors' => $errors,
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,34 +29,31 @@ public function __construct() {
* @return array
*/
public static function get_extensions( $allowed_bundles = array() ) {
$bundles = array();
$specs = self::get_specs();
$locale = get_user_locale();

foreach ( $specs as $spec ) {
$spec = (object) $spec;
$bundle = (array) $spec;
$bundle['plugins'] = array();
$specs = self::get_specs();
$results = EvaluateExtension::evaluate_bundles( $specs, $allowed_bundles );

if ( ! empty( $allowed_bundles ) && ! in_array( $spec->key, $allowed_bundles, true ) ) {
continue;
$plugins = array_filter(
$results['bundles'],
function( $bundle ) {
return count( $bundle['plugins'] ) > 0;
}
);

foreach ( $spec->plugins as $plugin ) {
try {
$extension = EvaluateExtension::evaluate( (object) $plugin );
if ( ! property_exists( $extension, 'is_visible' ) || $extension->is_visible ) {
$bundle['plugins'][] = $extension;
}
// phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch
} catch ( \Throwable $e ) {
// Ignore errors.
}
}
// When no plugins are visible, replace it with defaults and save for 3 hours.
if ( empty( $plugins ) ) {
ilyasfoo marked this conversation as resolved.
Show resolved Hide resolved
RemoteFreeExtensionsDataSourcePoller::get_instance()->set_specs_transient( array( $locale => DefaultFreeExtensions::get_all() ), 3 * HOUR_IN_SECONDS );

return EvaluateExtension::evaluate_bundles( DefaultFreeExtensions::get_all(), $allowed_bundles )['bundles'];
}

$bundles[] = $bundle;
// When plugins is not empty but has errors, save it for 3 hours.
if ( count( $results['errors'] ) > 0 ) {
RemoteFreeExtensionsDataSourcePoller::get_instance()->set_specs_transient( array( $locale => $specs ), 3 * HOUR_IN_SECONDS );
}

return $bundles;
return $results['bundles'];
}

/**
Expand Down
34 changes: 12 additions & 22 deletions plugins/woocommerce/src/Internal/Admin/WCPayPromotion/Init.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ public function __construct() {
}

add_filter( 'woocommerce_payment_gateways', array( __CLASS__, 'possibly_register_pre_install_wc_pay_promotion_gateway' ) );
add_filter( 'option_woocommerce_gateway_order', [ __CLASS__, 'set_gateway_top_of_list' ] );
add_filter( 'default_option_woocommerce_gateway_order', [ __CLASS__, 'set_gateway_top_of_list' ] );
add_filter( 'option_woocommerce_gateway_order', array( __CLASS__, 'set_gateway_top_of_list' ) );
add_filter( 'default_option_woocommerce_gateway_order', array( __CLASS__, 'set_gateway_top_of_list' ) );

$rtl = is_rtl() ? '.rtl' : '';

Expand Down Expand Up @@ -122,28 +122,18 @@ function( $promotion ) {
* Go through the specs and run them.
*/
public static function get_promotions() {
$suggestions = array();
$specs = self::get_specs();

foreach ( $specs as $spec ) {
try {
$suggestion = EvaluateSuggestion::evaluate( $spec );
$suggestions[] = $suggestion;
// phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch
} catch ( \Throwable $e ) {
// Ignore errors.
}
}
$locale = get_user_locale();

return array_values(
array_filter(
$suggestions,
function( $suggestion ) {
return ! property_exists( $suggestion, 'is_visible' ) || $suggestion->is_visible;
}
)
);
$specs = self::get_specs();
$results = EvaluateSuggestion::evaluate_specs( $specs );

if ( count( $results['errors'] ) > 0 ) {
// Unlike payment gateway suggestions, we don't have a non-empty default set of promotions to fall back to.
// So just set the specs transient with expired time to 3 hours.
WCPayPromotionDataSourcePoller::get_instance()->set_specs_transient( array( $locale => $specs ), 3 * HOUR_IN_SECONDS );
}

return $results['suggestions'];
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use Automattic\WooCommerce\Admin\Features\PaymentGatewaySuggestions\Init as PaymentGatewaySuggestions;
use Automattic\WooCommerce\Admin\Features\PaymentGatewaySuggestions\DefaultPaymentGateways;
use Automattic\WooCommerce\Admin\Features\PaymentGatewaySuggestions\PaymentGatewaySuggestionsDataSourcePoller;
use Automattic\WooCommerce\Admin\RemoteInboxNotifications\PassRuleProcessor;

/**
* class WC_Admin_Tests_PaymentGatewaySuggestions_Init
Expand Down Expand Up @@ -59,13 +60,21 @@ public function get_mock_specs() {
return array(
'en_US' => array(
array(
'id' => 'mock-gateway',
'id' => 'mock-gateway-1',
'is_visible' => (object) array(
'type' => 'base_location_country',
'value' => 'ZA',
'operation' => '=',
),
),
array(
'id' => 'mock-gateway-2',
'is_visible' => (object) array(
'type' => 'base_location_country',
'value' => 'US',
'operation' => '=',
),
),
),
);
}
Expand Down Expand Up @@ -111,27 +120,15 @@ public function test_specs_transient() {
/**
* Test that non-matched suggestions are not shown.
*/
public function test_non_matching_suggestions() {
update_option( 'woocommerce_default_country', 'US' );
set_transient(
'woocommerce_admin_' . PaymentGatewaySuggestionsDataSourcePoller::ID . '_specs',
$this->get_mock_specs()
);
$suggestions = PaymentGatewaySuggestions::get_suggestions();
$this->assertCount( 0, $suggestions );
}

/**
* Test that matched suggestions are shown.
*/
public function test_matching_suggestions() {
update_option( 'woocommerce_default_country', 'ZA' );
update_option( 'woocommerce_default_country', 'US' );
set_transient(
'woocommerce_admin_' . PaymentGatewaySuggestionsDataSourcePoller::ID . '_specs',
$this->get_mock_specs()
);
$suggestions = PaymentGatewaySuggestions::get_suggestions();
$this->assertEquals( 'mock-gateway', $suggestions[0]->id );
$this->assertCount( 1, $suggestions );
$this->assertEquals( 'mock-gateway-2', $suggestions[0]->id );
}

/**
Expand Down Expand Up @@ -165,6 +162,27 @@ function( $_locale ) {
$this->assertEquals( 'default-gateway', $suggestions[0]->id );
}

/**
* Test that empty suggestions are replaced with defaults.
*/
public function test_empty_suggestions() {
set_transient(
'woocommerce_admin_' . PaymentGatewaySuggestionsDataSourcePoller::ID . '_specs',
array(
'en_US' => array(),
)
);

$suggestions = PaymentGatewaySuggestions::get_suggestions();
$stored_transients = get_transient( 'woocommerce_admin_' . PaymentGatewaySuggestionsDataSourcePoller::ID . '_specs' );

$this->assertEquals( 'bacs', $suggestions[0]->id );
$this->assertEquals( count( $stored_transients['en_US'] ), count( DefaultPaymentGateways::get_all() ) );

$expires = (int) get_transient( '_transient_timeout_woocommerce_admin_' . PaymentGatewaySuggestionsDataSourcePoller::ID . '_specs' );
$this->assertTrue( ( $expires - time() ) < 3 * HOUR_IN_SECONDS );
Copy link
Contributor

Choose a reason for hiding this comment

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

I think changing it to <= might be safer to accommodate for supercomputers running unit tests 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed in 0ae2595. Thanks! 😆

}

/**
* Test that the suggestions can be displayed when a user has marketplace
* suggestions enabled and is a user capable of installing plugins.
Expand Down