From d71f4091492e7b398a66901110619a33b88de8f1 Mon Sep 17 00:00:00 2001 From: Marcin Bot Date: Mon, 2 Oct 2023 13:45:06 +0100 Subject: [PATCH] Add DisallowHooksInConstructor sniff and migrate some classes to use it (#7249) --- changelog/add-constructor-hooks-sniffer | 4 ++ .../DisallowHooksInConstructorSniff.php | 69 +++++++++++++++++++ dev/phpcs/ruleset.xml | 40 +++++++++++ ...s-wc-payments-admin-sections-overwrite.php | 7 ++ .../class-wc-payments-admin-settings.php | 7 ++ includes/admin/class-wc-payments-admin.php | 37 ++++++---- includes/class-database-cache.php | 7 ++ .../class-wc-payments-dependency-service.php | 7 +- includes/class-wc-payments-status.php | 7 ++ includes/class-wc-payments.php | 15 ++-- .../wc-payment-api/class-wc-payments-http.php | 7 ++ phpcs.xml.dist | 4 ++ ...s-wc-payments-admin-sections-overwrite.php | 4 +- 13 files changed, 192 insertions(+), 23 deletions(-) create mode 100644 changelog/add-constructor-hooks-sniffer create mode 100644 dev/phpcs/Sniffs/DisallowHooksInConstructorSniff.php create mode 100644 dev/phpcs/ruleset.xml diff --git a/changelog/add-constructor-hooks-sniffer b/changelog/add-constructor-hooks-sniffer new file mode 100644 index 00000000000..14e54f24c3c --- /dev/null +++ b/changelog/add-constructor-hooks-sniffer @@ -0,0 +1,4 @@ +Significance: patch +Type: dev + +Migrate away from hooking into actions in certain classes diff --git a/dev/phpcs/Sniffs/DisallowHooksInConstructorSniff.php b/dev/phpcs/Sniffs/DisallowHooksInConstructorSniff.php new file mode 100644 index 00000000000..519082159a7 --- /dev/null +++ b/dev/phpcs/Sniffs/DisallowHooksInConstructorSniff.php @@ -0,0 +1,69 @@ + + */ + public function register() { + return [ \T_FUNCTION ]; + } + + /** + * Processes the sniff if one of its tokens is encountered. + * + * @param File $phpcsFile The current file being checked. + * @param int $stackPtr The position of the current token in the stack passed in $tokens. + * + * @return void + */ + public function process( File $phpcsFile, $stackPtr ) { + $tokens = $phpcsFile->getTokens(); + + // Check that we're looking at the __construct function. + $namePtr = $phpcsFile->findNext( T_STRING, $stackPtr + 1 ); + $nameToken = $tokens[ $namePtr ]; + if ( $nameToken['content'] !== '__construct' ) { + return; + } + + // Retrieve the current token. + $token = $tokens[ $stackPtr ]; + + // Check the current token's scope. + $scopeOpener = $token['scope_opener']; + $scopeCloser = $token['scope_closer']; + + // For every string token in the scope... + for ( $i = $scopeOpener; $i < $scopeCloser; $i++ ) { + if ( $tokens[ $i ]['type'] !== 'T_STRING' ) { + continue; + } + // ...check if it's one of the forbidden functions. + $currentToken = $tokens[ $i ]; + $currentTokenContent = $currentToken['content']; + if ( in_array( $currentTokenContent, $this->forbiddenFunctions ) ) { + $phpcsFile->addError( + "Usage of $currentTokenContent in __construct() is not allowed", + $i, + 'WCPay.CodingStandards.DisallowHooksInConstructor', + [ $token['content'] ] + ); + } + } + } +} diff --git a/dev/phpcs/ruleset.xml b/dev/phpcs/ruleset.xml new file mode 100644 index 00000000000..af8e37e8ef7 --- /dev/null +++ b/dev/phpcs/ruleset.xml @@ -0,0 +1,40 @@ + + + WCPay custom coding standards. + + + */includes/multi-currency/* + + + */includes/subscriptions/* + */includes/compat/subscriptions/* + + + */includes/emails/* + + + */includes/class-woopay-tracker.php + */includes/woopay/* + */includes/woopay-user/* + */includes/class-wc-payments-order-success-page.php + + + */includes/class-wc-payments-fraud-service.php + */includes/fraud-prevention/* + + + */includes/class-wc-payments-account.php + */includes/class-wc-payments-incentives-service.php + */includes/class-wc-payments-onboarding-service.php + + + */includes/class-wc-payments-apple-pay-registration.php + */includes/class-wc-payments-express-checkout-button-display-handler.php + + + */includes/class-wc-payments-customer-service.php + */includes/class-wc-payments-token-service.php + + + */includes/class-wc-payments-webhook-reliability-service.php + diff --git a/includes/admin/class-wc-payments-admin-sections-overwrite.php b/includes/admin/class-wc-payments-admin-sections-overwrite.php index 29d45e4bed8..ca46c93a887 100644 --- a/includes/admin/class-wc-payments-admin-sections-overwrite.php +++ b/includes/admin/class-wc-payments-admin-sections-overwrite.php @@ -25,7 +25,14 @@ class WC_Payments_Admin_Sections_Overwrite { */ public function __construct( WC_Payments_Account $account ) { $this->account = $account; + } + /** + * Initializes this class's WP hooks. + * + * @return void + */ + public function init_hooks() { add_filter( 'woocommerce_get_sections_checkout', [ $this, 'add_checkout_sections' ] ); } diff --git a/includes/admin/class-wc-payments-admin-settings.php b/includes/admin/class-wc-payments-admin-settings.php index 09796f6eaa7..49a062a57ee 100644 --- a/includes/admin/class-wc-payments-admin-settings.php +++ b/includes/admin/class-wc-payments-admin-settings.php @@ -35,7 +35,14 @@ class WC_Payments_Admin_Settings { */ public function __construct( WC_Payment_Gateway_WCPay $gateway ) { $this->gateway = $gateway; + } + /** + * Initializes this class's WP hooks. + * + * @return void + */ + public function init_hooks() { add_action( 'woocommerce_woocommerce_payments_admin_notices', [ $this, 'display_test_mode_notice' ] ); add_filter( 'plugin_action_links_' . plugin_basename( WCPAY_PLUGIN_FILE ), [ $this, 'add_plugin_links' ] ); } diff --git a/includes/admin/class-wc-payments-admin.php b/includes/admin/class-wc-payments-admin.php index 477a9c8ccfe..d76a2dae6c8 100644 --- a/includes/admin/class-wc-payments-admin.php +++ b/includes/admin/class-wc-payments-admin.php @@ -136,21 +136,6 @@ public function __construct( $this->incentives_service = $incentives_service; $this->database_cache = $database_cache; - add_action( 'admin_notices', [ $this, 'display_not_supported_currency_notice' ], 9999 ); - add_action( 'admin_notices', [ $this, 'display_isk_decimal_notice' ] ); - - add_action( 'woocommerce_admin_order_data_after_payment_info', [ $this, 'render_order_edit_payment_details_container' ] ); - - // Add menu items. - add_action( 'admin_menu', [ $this, 'add_payments_menu' ], 0 ); - add_action( 'admin_init', [ $this, 'maybe_redirect_to_onboarding' ], 11 ); // Run this after the WC setup wizard and onboarding redirection logic. - add_action( 'admin_enqueue_scripts', [ $this, 'maybe_redirect_overview_to_connect' ], 1 ); // Run this late (after `admin_init`) but before any scripts are actually enqueued. - add_action( 'admin_enqueue_scripts', [ $this, 'register_payments_scripts' ], 9 ); - add_action( 'admin_enqueue_scripts', [ $this, 'enqueue_payments_scripts' ], 9 ); - add_action( 'woocommerce_admin_field_payment_gateways', [ $this, 'payment_gateways_container' ] ); - add_action( 'woocommerce_admin_order_totals_after_total', [ $this, 'show_woopay_payment_method_name_admin' ] ); - add_action( 'woocommerce_admin_order_totals_after_total', [ $this, 'display_wcpay_transaction_fee' ] ); - $this->admin_child_pages = [ 'wc-payments-overview' => [ 'id' => 'wc-payments-overview', @@ -195,6 +180,28 @@ public function __construct( ]; } + /** + * Initializes this class's WP hooks. + * + * @return void + */ + public function init_hooks() { + add_action( 'admin_notices', [ $this, 'display_not_supported_currency_notice' ], 9999 ); + add_action( 'admin_notices', [ $this, 'display_isk_decimal_notice' ] ); + + add_action( 'woocommerce_admin_order_data_after_payment_info', [ $this, 'render_order_edit_payment_details_container' ] ); + + // Add menu items. + add_action( 'admin_menu', [ $this, 'add_payments_menu' ], 0 ); + add_action( 'admin_init', [ $this, 'maybe_redirect_to_onboarding' ], 11 ); // Run this after the WC setup wizard and onboarding redirection logic. + add_action( 'admin_enqueue_scripts', [ $this, 'maybe_redirect_overview_to_connect' ], 1 ); // Run this late (after `admin_init`) but before any scripts are actually enqueued. + add_action( 'admin_enqueue_scripts', [ $this, 'register_payments_scripts' ], 9 ); + add_action( 'admin_enqueue_scripts', [ $this, 'enqueue_payments_scripts' ], 9 ); + add_action( 'woocommerce_admin_field_payment_gateways', [ $this, 'payment_gateways_container' ] ); + add_action( 'woocommerce_admin_order_totals_after_total', [ $this, 'show_woopay_payment_method_name_admin' ] ); + add_action( 'woocommerce_admin_order_totals_after_total', [ $this, 'display_wcpay_transaction_fee' ] ); + } + /** * Add notice explaining that the selected currency is not available. */ diff --git a/includes/class-database-cache.php b/includes/class-database-cache.php index 0308c90b2e7..fd6d6290eea 100644 --- a/includes/class-database-cache.php +++ b/includes/class-database-cache.php @@ -79,7 +79,14 @@ class Database_Cache { */ public function __construct() { $this->refresh_disabled = false; + } + /** + * Initializes this class's WP hooks. + * + * @return void + */ + public function init_hooks() { add_action( 'action_scheduler_before_execute', [ $this, 'disable_refresh' ] ); } diff --git a/includes/class-wc-payments-dependency-service.php b/includes/class-wc-payments-dependency-service.php index 0eb66a09faa..c952c20113b 100644 --- a/includes/class-wc-payments-dependency-service.php +++ b/includes/class-wc-payments-dependency-service.php @@ -25,10 +25,11 @@ class WC_Payments_Dependency_Service { const DEV_ASSETS_NOT_BUILT = 'dev_assets_not_built'; /** - * Constructor. + * Initializes this class's WP hooks. + * + * @return void */ - public function __construct() { - + public function init_hooks() { add_filter( 'admin_notices', [ $this, 'display_admin_notices' ] ); } diff --git a/includes/class-wc-payments-status.php b/includes/class-wc-payments-status.php index 868d3f441dd..02f4977df63 100644 --- a/includes/class-wc-payments-status.php +++ b/includes/class-wc-payments-status.php @@ -45,7 +45,14 @@ public function __construct( $gateway, $http, $account ) { $this->gateway = $gateway; $this->http = $http; $this->account = $account; + } + /** + * Initializes this class's WP hooks. + * + * @return void + */ + public function init_hooks() { add_action( 'woocommerce_system_status_report', [ $this, 'render_status_report_section' ], 1 ); add_filter( 'woocommerce_debug_tools', [ $this, 'debug_tools' ] ); } diff --git a/includes/class-wc-payments.php b/includes/class-wc-payments.php index 124b42bd31e..539f102e5fc 100644 --- a/includes/class-wc-payments.php +++ b/includes/class-wc-payments.php @@ -297,10 +297,12 @@ public static function init() { include_once __DIR__ . '/class-database-cache.php'; self::$database_cache = new Database_Cache(); + self::$database_cache->init_hooks(); include_once __DIR__ . '/class-wc-payments-dependency-service.php'; self::$dependency_service = new WC_Payments_Dependency_Service(); + self::$dependency_service->init_hooks(); if ( false === self::$dependency_service->has_valid_dependencies() ) { return; @@ -601,7 +603,7 @@ public static function init() { } if ( is_admin() && current_user_can( 'manage_woocommerce' ) ) { - new WC_Payments_Admin( + $admin = new WC_Payments_Admin( self::$api_client, self::get_gateway(), self::$account, @@ -610,16 +612,20 @@ public static function init() { self::$incentives_service, self::$database_cache ); + $admin->init_hooks(); - new WC_Payments_Admin_Settings( self::get_gateway() ); + $admin_settings = new WC_Payments_Admin_Settings( self::get_gateway() ); + $admin_settings->init_hooks(); // Use tracks loader only in admin screens because it relies on WC_Tracks loaded by WC_Admin. include_once WCPAY_ABSPATH . 'includes/admin/tracks/tracks-loader.php'; include_once __DIR__ . '/admin/class-wc-payments-admin-sections-overwrite.php'; - new WC_Payments_Admin_Sections_Overwrite( self::get_account_service() ); + $admin_sections_overwrite = new WC_Payments_Admin_Sections_Overwrite( self::get_account_service() ); + $admin_sections_overwrite->init_hooks(); - new WC_Payments_Status( self::get_gateway(), self::get_wc_payments_http(), self::get_account_service() ); + $wcpay_status = new WC_Payments_Status( self::get_gateway(), self::get_wc_payments_http(), self::get_account_service() ); + $wcpay_status->init_hooks(); new WCPay\Fraud_Prevention\Order_Fraud_And_Risk_Meta_Box( self::$order_service ); } @@ -945,6 +951,7 @@ private static function get_wc_payments_http() { if ( ! $http_class instanceof WC_Payments_Http_Interface ) { $http_class = new WC_Payments_Http( new Automattic\Jetpack\Connection\Manager( 'woocommerce-payments' ) ); + $http_class->init_hooks(); } return $http_class; diff --git a/includes/wc-payment-api/class-wc-payments-http.php b/includes/wc-payment-api/class-wc-payments-http.php index 552faad5c8e..a812406070e 100644 --- a/includes/wc-payment-api/class-wc-payments-http.php +++ b/includes/wc-payment-api/class-wc-payments-http.php @@ -32,7 +32,14 @@ class WC_Payments_Http implements WC_Payments_Http_Interface { */ public function __construct( $connection_manager ) { $this->connection_manager = $connection_manager; + } + /** + * Initializes this class's WP hooks. + * + * @return void + */ + public function init_hooks() { add_filter( 'allowed_redirect_hosts', [ $this, 'allowed_redirect_hosts' ] ); } diff --git a/phpcs.xml.dist b/phpcs.xml.dist index 9f8efd70ee5..d99908d97b3 100644 --- a/phpcs.xml.dist +++ b/phpcs.xml.dist @@ -7,6 +7,7 @@ + ./dev/* ./dist/* ./release/* ./docker/* @@ -70,4 +71,7 @@ + + + diff --git a/tests/unit/admin/test-class-wc-payments-admin-sections-overwrite.php b/tests/unit/admin/test-class-wc-payments-admin-sections-overwrite.php index 47ec5c73bef..3a308511249 100644 --- a/tests/unit/admin/test-class-wc-payments-admin-sections-overwrite.php +++ b/tests/unit/admin/test-class-wc-payments-admin-sections-overwrite.php @@ -47,7 +47,9 @@ public function test_checkout_sections_are_modified() { ->expects( $this->any() ) ->method( 'get_cached_account_data' ) ->willReturn( [ 'is_live' => true ] ); - new WC_Payments_Admin_Sections_Overwrite( $this->account_service ); + + $admin_sections_overwrite = new WC_Payments_Admin_Sections_Overwrite( $this->account_service ); + $admin_sections_overwrite->init_hooks(); $this->assertEquals( $expected_sections,