Add DisallowHooksInConstructor sniff and migrate some classes to use …

…it (#7249)

changelog/add-constructor-hooks-sniffer

@@ -0,0 +1,4 @@
+Significance: patch
+Type: dev
+
+Migrate away from hooking into actions in certain classes

dev/phpcs/Sniffs/DisallowHooksInConstructorSniff.php

@@ -0,0 +1,69 @@
+<?php
+/**
+ * This sniff prohibits the use of add_action and add_filter in __construct.
+ */
+
+namespace WCPay\CodingStandards\Sniffs;
+
+use PHP_CodeSniffer\Sniffs\Sniff;
+use PHP_CodeSniffer\Files\File;
+
+class DisallowHooksInConstructorSniff implements Sniff {
+	private $forbiddenFunctions = [
+		'add_action',
+		'add_filter',
+	];
+
+	/**
+	 * Returns the token types that this sniff is interested in.
+	 *
+	 * @return array<int>
+	 */
+	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'] ]
+				);
+			}
+		}
+	}
+}

dev/phpcs/ruleset.xml

@@ -0,0 +1,40 @@
+<?xml version="1.0"?>
+<ruleset name="WCPay" namespace="WCPay\CodingStandards">
+	<description>WCPay custom coding standards.</description>
+
+	<!-- https://github.com/Automattic/woocommerce-payments/issues/7258 -->
+	<exclude-pattern>*/includes/multi-currency/*</exclude-pattern>
+
+	<!-- https://github.com/Automattic/woocommerce-payments/issues/7259 -->
+	<exclude-pattern>*/includes/subscriptions/*</exclude-pattern>
+	<exclude-pattern>*/includes/compat/subscriptions/*</exclude-pattern>
+
+	<!-- https://github.com/Automattic/woocommerce-payments/issues/7266 -->
+	<exclude-pattern>*/includes/emails/*</exclude-pattern>
+
+	<!-- https://github.com/Automattic/woocommerce-payments/issues/7260 -->
+	<exclude-pattern>*/includes/class-woopay-tracker.php</exclude-pattern>
+	<exclude-pattern>*/includes/woopay/*</exclude-pattern>
+	<exclude-pattern>*/includes/woopay-user/*</exclude-pattern>
+	<exclude-pattern>*/includes/class-wc-payments-order-success-page.php</exclude-pattern>
+
+	<!-- https://github.com/Automattic/woocommerce-payments/issues/7261 -->
+	<exclude-pattern>*/includes/class-wc-payments-fraud-service.php</exclude-pattern>
+	<exclude-pattern>*/includes/fraud-prevention/*</exclude-pattern>
+
+	<!-- https://github.com/Automattic/woocommerce-payments/issues/7262 -->
+	<exclude-pattern>*/includes/class-wc-payments-account.php</exclude-pattern>
+	<exclude-pattern>*/includes/class-wc-payments-incentives-service.php</exclude-pattern>
+	<exclude-pattern>*/includes/class-wc-payments-onboarding-service.php</exclude-pattern>
+
+	<!-- https://github.com/Automattic/woocommerce-payments/issues/7263 -->
+	<exclude-pattern>*/includes/class-wc-payments-apple-pay-registration.php</exclude-pattern>
+	<exclude-pattern>*/includes/class-wc-payments-express-checkout-button-display-handler.php</exclude-pattern>
+
+	<!-- https://github.com/Automattic/woocommerce-payments/issues/7264 -->
+	<exclude-pattern>*/includes/class-wc-payments-customer-service.php</exclude-pattern>
+	<exclude-pattern>*/includes/class-wc-payments-token-service.php</exclude-pattern>
+
+	<!-- https://github.com/Automattic/woocommerce-payments/issues/7265 -->
+	<exclude-pattern>*/includes/class-wc-payments-webhook-reliability-service.php</exclude-pattern>
+</ruleset>

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' ] );
 	}
 

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' ] );
 	}

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.
 	 */

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' ] );
 	}
 

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' ] );
 	}
 

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' ] );
 	}

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;

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' ] );
 	}
 

phpcs.xml.dist

@@ -7,6 +7,7 @@
 	<arg name="extensions" value="php"/>
 
     <!-- Exclude paths -->
+	<exclude-pattern>./dev/*</exclude-pattern>
 	<exclude-pattern>./dist/*</exclude-pattern>
 	<exclude-pattern>./release/*</exclude-pattern>
 	<exclude-pattern>./docker/*</exclude-pattern>
@@ -70,4 +71,7 @@
 	</rule>
 
 	<rule ref="PEAR.WhiteSpace.ObjectOperatorIndent" />
+
+	<!-- Custom WCPay rules -->
+	<rule ref="./dev/phpcs/ruleset.xml"/>
 </ruleset>

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,