From c99e4c995380e4d15bf6c261d774757cf417872a Mon Sep 17 00:00:00 2001 From: Igor Zinovyev Date: Fri, 10 Jan 2020 20:12:39 +0300 Subject: [PATCH] Trying to add deterministic initialization. (#14168) * Trying to add deterministic initialization. This change tries several new ways to initialize objects by using hooks instead of initalizing manually. * Fixed the order in which init happens, fixed tests. * Fixes standalone sync suite test run and removes redundant init call. * Added a config package to assist with initialization. * Added basic configuration to the config package. * Additional init parameter for the Actions class. * Brought the init call back because we need at least one, props @kbrown9. * Reverted Sync changes related to passing instances. * Rollback and taking a different direction: separate instances where possible, feature requirement instead of configuration. * Added a Jetpack::configure call to the bootstrap, props @kbrown9 for the fix! * Added a README file for the config package. * Update composer.lock after rebasing against master * Refactored the way we initalize classes and added one time init protection. * Added a README part about adding your package to the Config class. * Updated priority number in the README, props @kbrown9 for spotting that! * Fixed the comment. * Fixed the ensure_class method, removed the feature variable that is no longer available. * Added feature ensure constants to make ensure call return values more logical. * Updated the version number in the filter comment. Co-authored-by: Brandon Kraft --- class.jetpack.php | 65 ++++--- composer.json | 1 + composer.lock | 22 ++- load-jetpack.php | 2 - packages/autoloader/src/autoload.php | 1 - packages/config/README.md | 126 +++++++++++++ packages/config/composer.json | 13 ++ packages/config/src/class-config.php | 169 ++++++++++++++++++ packages/sync/src/class-listener.php | 1 - packages/sync/src/class-main.php | 48 +++-- packages/sync/src/class-sender.php | 4 +- packages/sync/src/class-users.php | 23 +-- tests/php/bootstrap.php | 1 + tests/php/sync/class.silent-upgrader-skin.php | 2 + .../php/sync/test_class.jetpack-sync-base.php | 2 - .../sync/test_class.jetpack-sync-options.php | 8 - 16 files changed, 419 insertions(+), 69 deletions(-) create mode 100644 packages/config/README.md create mode 100644 packages/config/composer.json create mode 100644 packages/config/src/class-config.php diff --git a/class.jetpack.php b/class.jetpack.php index 1810509071697..57d06560c19e0 100644 --- a/class.jetpack.php +++ b/class.jetpack.php @@ -1,10 +1,9 @@ has_agreed() ) { - add_action( 'init', array( $tracking, 'init' ) ); - } else { - /** - * Initialize tracking right after the user agrees to the terms of service. - */ - add_action( 'jetpack_agreed_to_terms_of_service', array( $tracking, 'init' ) ); + foreach ( + array( + 'sync', + 'tracking', + 'tos', + ) + as $feature + ) { + $config->ensure( $feature ); + } + + /* + * Enable enhanced handling of previewing sites in Calypso + */ + if ( self::is_active() ) { + require_once JETPACK__PLUGIN_DIR . '_inc/lib/class.jetpack-iframe-embed.php'; + add_action( 'init', array( 'Jetpack_Iframe_Embed', 'init' ), 9, 0 ); + require_once JETPACK__PLUGIN_DIR . '_inc/lib/class.jetpack-keyring-service-helper.php'; + add_action( 'init', array( 'Jetpack_Keyring_Service_Helper', 'init' ), 9, 0 ); } - add_filter( 'map_meta_cap', array( $this, 'jetpack_custom_caps' ), 1, 4 ); } /** @@ -763,6 +760,11 @@ function after_plugins_loaded() { * @action plugins_loaded */ public function late_initialization() { + self::plugin_textdomain(); + self::load_modules(); + + Partner::init(); + /** * Fires when Jetpack is fully loaded and ready. This is the point where it's safe * to instantiate classes from packages and namespaces that are managed by the Jetpack Autoloader. @@ -772,6 +774,8 @@ public function late_initialization() { * @param Jetpack $jetpack the main plugin class object. */ do_action( 'jetpack_loaded', $this ); + + add_filter( 'map_meta_cap', array( $this, 'jetpack_custom_caps' ), 1, 4 ); } /** @@ -5129,6 +5133,11 @@ public static function xmlrpc_api_url() { return self::connection()->xmlrpc_api_url(); } + /** + * Returns the connection manager object. + * + * @return Automattic\Jetpack\Connection\Manager + */ public static function connection() { return self::init()->connection_manager; } diff --git a/composer.json b/composer.json index 3007479ecc17a..e21cef24718f9 100644 --- a/composer.json +++ b/composer.json @@ -16,6 +16,7 @@ "automattic/jetpack-autoloader": "@dev", "automattic/jetpack-backup": "@dev", "automattic/jetpack-compat": "@dev", + "automattic/jetpack-config": "@dev", "automattic/jetpack-connection": "@dev", "automattic/jetpack-constants": "@dev", "automattic/jetpack-error": "@dev", diff --git a/composer.lock b/composer.lock index 1cfd863b44aa9..70335c32122cd 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "23a3df0c752fcfb8b0a94250870ae7b9", + "content-hash": "9eb6218b5e3e36832a66b69ce23b6882", "packages": [ { "name": "automattic/jetpack-abtest", @@ -159,6 +159,25 @@ ], "description": "Compatibility layer with previous versions of Jetpack" }, + { + "name": "automattic/jetpack-config", + "version": "dev-add/deterministic-init", + "dist": { + "type": "path", + "url": "./packages/config", + "reference": "09edd078205d6fee2f8a46eaf0f736fd4ade1473" + }, + "type": "library", + "autoload": { + "classmap": [ + "src/" + ] + }, + "license": [ + "GPL-2.0-or-later" + ], + "description": "Jetpack configuration package that initializes other packages and configures Jetpack's functionality. Can be used as a base for all variants of Jetpack package usage." + }, { "name": "automattic/jetpack-connection", "version": "dev-retry/phpcs-changed", @@ -968,6 +987,7 @@ "automattic/jetpack-autoloader": 20, "automattic/jetpack-backup": 20, "automattic/jetpack-compat": 20, + "automattic/jetpack-config": 20, "automattic/jetpack-connection": 20, "automattic/jetpack-constants": 20, "automattic/jetpack-error": 20, diff --git a/load-jetpack.php b/load-jetpack.php index e107afa72f122..d0cc05a3078d8 100644 --- a/load-jetpack.php +++ b/load-jetpack.php @@ -62,8 +62,6 @@ function jetpack_should_use_minified_assets() { require_once JETPACK__PLUGIN_DIR . 'class.jetpack-connection-banner.php'; require_once JETPACK__PLUGIN_DIR . 'class.jetpack-plan.php'; -Automattic\Jetpack\Sync\Main::init(); - if ( is_admin() ) { require_once JETPACK__PLUGIN_DIR . 'class.jetpack-admin.php'; $jitm = new Automattic\Jetpack\JITM(); diff --git a/packages/autoloader/src/autoload.php b/packages/autoloader/src/autoload.php index 839ea94bc2ed3..0794a68d028c6 100644 --- a/packages/autoloader/src/autoload.php +++ b/packages/autoloader/src/autoload.php @@ -83,7 +83,6 @@ function autoloader( $class_name ) { 'Jetpack_Options', 'Jetpack_Signature', 'Jetpack_XMLRPC_Server', - 'Automattic\Jetpack\Sync\Main', 'Automattic\Jetpack\Constants', 'Automattic\Jetpack\Tracking', ), diff --git a/packages/config/README.md b/packages/config/README.md new file mode 100644 index 0000000000000..7663669715151 --- /dev/null +++ b/packages/config/README.md @@ -0,0 +1,126 @@ +# Jetpack Configuration + +Allows for enabling and initializing of Jetpack features provided by +other packages. + +# Usage + +Add this package as a dependency to your project: + +``` +composer require automattic/jetpack-config +``` + +Add every other package you're planning to configure: + +``` +composer require automattic/jetpack-sync +composer require automattic/jetpack-tracking +composer require automattic/jetpack-terms-of-service +``` + +In your code initialize the configuration package at or before +plugins_loaded priority 1: + +``` +use Automattic/Jetpack/Config; + +// Configuring Jetpack as early as plugins_loaded priority 1 +// to make sure every action handler gets properly set. +add_action( 'plugins_loaded', 'configure_jetpack', 1 ); + +function configure_jetpack() { + $config = new Config(); + + foreach ( + array( + 'sync', + 'tracking', + 'tos', + ) + as $feature + ) { + $config->ensure( $feature ); + } +} +``` + +# Adding your package to the config class + +You can have your package initialized using the Config class by +adding several things. + +## The configure method + +It's better to have one static configure method in your package +class. That method will be called early on the `plugins_loaded` +hook. This way you can add your own `plugins_loaded` handlers with +standard priority and they will get executed: + +``` +class Configurable_Package { + + public static function configure() { + add_action( 'plugins_loaded', array( __CLASS__, 'on_plugins_loaded' ); + } + + public static function on_plugins_loaded() { + self::do_interesting_stuff(); + } + +} +``` + +## The feature enabling method + +An enabling method should be added to the Config class and should only contain your configuration method call. + +``` + +public function enable_configurable_package() { + Configurable_Package::configure(); + + return true; +} +``` + +Note that the method name should use the feature slug, in this case +your feature slug is `configurable_package` for the sake of +simplicity. When you're adding your feature it should be unique and +recognizable, like `sync` or `tracking`. + +## The feature slug + +To make sure the feature is supported by the Config class, you need to +add its slug to the config class property: + +``` + /** + * The initial setting values. + * + * @var Array + */ + protected $config = array( + // ... + 'configurable_package' => false, + // ... + ); +``` + +## The ensure call + +Finally you need to add a block that will check if your package is +loaded and mark it to be initialized: + +``` +if ( $this->config['configurable_package'] ) { + $this->ensure_class( 'Configurable_Package' ) && $this->ensure_feature( 'configurable_package' ); +} +``` + +This code does three things: it checks whether the current setup has +requested your package to be loaded. Next it checks if the class that +you need for the package to run is present, and then it adds the hook +handlers that initialize your class. After that you can use the config +package's interface in a Jetpack package consumer application and load +your package as shown in the first section of this README. diff --git a/packages/config/composer.json b/packages/config/composer.json new file mode 100644 index 0000000000000..9dbde9bced518 --- /dev/null +++ b/packages/config/composer.json @@ -0,0 +1,13 @@ +{ + "name": "automattic/jetpack-config", + "description": "Jetpack configuration package that initializes other packages and configures Jetpack's functionality. Can be used as a base for all variants of Jetpack package usage.", + "type": "library", + "license": "GPL-2.0-or-later", + "minimum-stability": "dev", + "require": {}, + "autoload": { + "classmap": [ + "src/" + ] + } +} diff --git a/packages/config/src/class-config.php b/packages/config/src/class-config.php new file mode 100644 index 0000000000000..05a16b735adb1 --- /dev/null +++ b/packages/config/src/class-config.php @@ -0,0 +1,169 @@ + false, + 'tracking' => false, + 'tos' => false, + ); + + /** + * Creates the configuration class instance. + */ + public function __construct() { + + /** + * Adding the config handler to run on priority 2 because the class itself is + * being constructed on priority 1. + */ + add_action( 'plugins_loaded', array( $this, 'on_plugins_loaded' ), 2 ); + } + + /** + * Require a feature to be initialized. It's up to the package consumer to actually add + * the package to their composer project. Declaring a requirement using this method + * instructs the class to initalize it. + * + * @param String $feature the feature slug. + */ + public function ensure( $feature ) { + $this->config[ $feature ] = true; + } + + /** + * Runs on plugins_loaded hook priority with priority 2. + * + * @action plugins_loaded + */ + public function on_plugins_loaded() { + if ( $this->config['tracking'] ) { + + $this->ensure_class( 'Automattic\Jetpack\Terms_Of_Service' ) + && $this->ensure_class( 'Automattic\Jetpack\Tracking' ) + && $this->ensure_feature( 'tracking' ); + } + + if ( $this->config['sync'] ) { + $this->ensure_class( 'Automattic\Jetpack\Sync\Main' ) + && $this->ensure_feature( 'sync' ); + } + } + + /** + * Returns true if the required class is available and alerts the user if it's not available + * in case the site is in debug mode. + * + * @param String $classname a fully qualified class name. + * @return Boolean whether the class is available. + */ + protected function ensure_class( $classname ) { + $available = class_exists( $classname ); + + if ( ! $available && defined( 'WP_DEBUG' ) && WP_DEBUG ) { + trigger_error( // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error + sprintf( + /* translators: %1$s is a PHP class name. */ + esc_html__( + 'Unable to load class %1$s. Please add the package that contains it using composer and make sure you are requiring the Jetpack autoloader', + 'jetpack' + ), + esc_html( $classname ) + ), + E_USER_NOTICE + ); + } + + return $available; + } + + /** + * Ensures a feature is enabled, sets it up if it hasn't already been set up. + * + * @param String $feature slug of the feature. + * @return Integer either FEATURE_ENSURED, FEATURE_ALREADY_ENSURED or FEATURE_NOT_AVAILABLE constants. + */ + protected function ensure_feature( $feature ) { + $method = 'enable_' . $feature; + if ( ! method_exists( $this, $method ) ) { + return self::FEATURE_NOT_AVAILABLE; + } + + if ( did_action( 'jetpack_feature_' . $feature . '_enabled' ) ) { + return self::FEATURE_ALREADY_ENSURED; + } + + $this->{ $method }(); + + /** + * Fires when a specific Jetpack package feature is initalized using the Config package. + * + * @since 8.2.0 + */ + do_action( 'jetpack_feature_' . $feature . '_enabled' ); + + return self::FEATURE_ENSURED; + } + + /** + * Dummy method to enable Terms of Service. + */ + protected function enable_tos() { + return true; + } + + /** + * Enables the tracking feature. Depends on the Terms of Service package, so enables it too. + */ + protected function enable_tracking() { + + // Enabling dependencies. + $this->ensure_feature( 'tos' ); + + $terms_of_service = new Terms_Of_Service(); + $tracking = new Plugin_Tracking(); + if ( $terms_of_service->has_agreed() ) { + add_action( 'init', array( $tracking, 'init' ) ); + } else { + /** + * Initialize tracking right after the user agrees to the terms of service. + */ + add_action( 'jetpack_agreed_to_terms_of_service', array( $tracking, 'init' ) ); + } + + return true; + } + + /** + * Enables the Sync feature. + */ + protected function enable_sync() { + Sync_Main::configure(); + + return true; + } + +} diff --git a/packages/sync/src/class-listener.php b/packages/sync/src/class-listener.php index 8073e11b87f79..db5f377e00009 100644 --- a/packages/sync/src/class-listener.php +++ b/packages/sync/src/class-listener.php @@ -70,7 +70,6 @@ public static function get_instance() { * This is necessary because you can't use "new" when you declare instance properties >:( */ protected function __construct() { - Main::init(); $this->set_defaults(); $this->init(); } diff --git a/packages/sync/src/class-main.php b/packages/sync/src/class-main.php index 2e1c3cbd486d4..c149be57ea41f 100644 --- a/packages/sync/src/class-main.php +++ b/packages/sync/src/class-main.php @@ -7,28 +7,56 @@ namespace Automattic\Jetpack\Sync; +use Automattic\Jetpack\Sync\Actions as Sync_Actions; + /** * Jetpack Sync main class. */ class Main { + + /** + * Sets up event handlers for the Sync package. Is used from the Config package. + * + * @action plugins_loaded + */ + public static function configure() { + add_action( 'plugins_loaded', array( __CLASS__, 'on_plugins_loaded_early' ), 5 ); + add_action( 'plugins_loaded', array( __CLASS__, 'on_plugins_loaded_late' ), 90 ); + } + /** * Initialize the main sync actions. + * + * @action plugins_loaded */ - public static function init() { - // Check for WooCommerce support. - add_action( 'plugins_loaded', array( 'Automattic\\Jetpack\\Sync\\Actions', 'initialize_woocommerce' ), 5 ); + public static function on_plugins_loaded_early() { + /** + * Additional Sync modules can be carried out into their own packages and they + * will get their own config settings. + * + * For now additional modules are enabled based on whether the third party plugin + * class exists or not. + */ + Sync_Actions::initialize_woocommerce(); + Sync_Actions::initialize_wp_super_cache(); - // Check for WP Super Cache. - add_action( 'plugins_loaded', array( 'Automattic\\Jetpack\\Sync\\Actions', 'initialize_wp_super_cache' ), 5 ); + // We need to define this here so that it's hooked before `updating_jetpack_version` is called. + add_action( 'updating_jetpack_version', array( 'Automattic\\Jetpack\\Sync\\Actions', 'cleanup_on_upgrade' ), 10, 2 ); + add_action( 'jetpack_user_authorized', array( 'Automattic\\Jetpack\\Sync\\Actions', 'do_initial_sync' ), 10, 0 ); + } + /** + * Runs after most of plugins_loaded hook functions have been run. + * + * @action plugins_loaded + */ + public static function on_plugins_loaded_late() { /* * Init after plugins loaded and before the `init` action. This helps with issues where plugins init * with a high priority or sites that use alternate cron. */ - add_action( 'plugins_loaded', array( 'Automattic\\Jetpack\\Sync\\Actions', 'init' ), 90 ); - - // We need to define this here so that it's hooked before `updating_jetpack_version` is called. - add_action( 'updating_jetpack_version', array( 'Automattic\\Jetpack\\Sync\\Actions', 'cleanup_on_upgrade' ), 10, 2 ); - add_action( 'jetpack_user_authorized', array( 'Automattic\\Jetpack\\Sync\\Actions', 'do_initial_sync' ), 10, 0 ); + Sync_Actions::init(); } + + } diff --git a/packages/sync/src/class-sender.php b/packages/sync/src/class-sender.php index 923451e3d4f6a..f34ed67fb1c4c 100644 --- a/packages/sync/src/class-sender.php +++ b/packages/sync/src/class-sender.php @@ -7,6 +7,7 @@ namespace Automattic\Jetpack\Sync; +use Automattic\Jetpack\Connection\Manager; use Automattic\Jetpack\Constants; /** @@ -199,7 +200,8 @@ private function init() { * @access public */ public function maybe_set_user_from_token() { - $verified_user = \Jetpack::connection()->verify_xml_rpc_signature(); + $connection = new Manager(); + $verified_user = $connection->verify_xml_rpc_signature(); if ( Constants::is_true( 'XMLRPC_REQUEST' ) && ! is_wp_error( $verified_user ) && $verified_user diff --git a/packages/sync/src/class-users.php b/packages/sync/src/class-users.php index efb43a28faa0b..f37492f68add1 100644 --- a/packages/sync/src/class-users.php +++ b/packages/sync/src/class-users.php @@ -26,16 +26,6 @@ class Users { */ public static $user_roles = array(); - /** - * Jetpack connection manager instance. - * - * @access public - * @static - * - * @var null|Automattic\Jetpack\Connection\Manager - */ - public static $connection = null; - /** * Initialize sync for user data changes. * @@ -44,8 +34,8 @@ class Users { * @todo Eventually, connection needs to be instantiated at the top level in the sync package. */ public static function init() { - self::$connection = new Jetpack_Connection(); - if ( self::$connection->is_active() ) { + $connection = new Jetpack_Connection(); + if ( $connection->is_active() ) { // Kick off synchronization of user role when it changes. add_action( 'set_user_role', array( __CLASS__, 'user_role_change' ) ); } @@ -60,7 +50,8 @@ public static function init() { * @param int $user_id ID of the user. */ public static function user_role_change( $user_id ) { - if ( self::$connection->is_user_connected( $user_id ) ) { + $connection = new Jetpack_Connection(); + if ( $connection->is_user_connected( $user_id ) ) { self::update_role_on_com( $user_id ); // Try to choose a new master if we're demoting the current one. self::maybe_demote_master_user( $user_id ); @@ -101,7 +92,8 @@ public static function get_role( $user_id ) { * @return string Signed role of the user. */ public static function get_signed_role( $user_id ) { - return \Jetpack::connection()->sign_role( self::get_role( $user_id ), $user_id ); + $connection = new Jetpack_Connection(); + return $connection->sign_role( self::get_role( $user_id ), $user_id ); } /** @@ -140,9 +132,10 @@ public static function maybe_demote_master_user( $user_id ) { ) ); $new_master = false; + $connection = new Jetpack_Connection(); foreach ( $query->results as $result ) { $found_user_id = absint( $result->id ); - if ( $found_user_id && self::$connection->is_user_connected( $found_user_id ) ) { + if ( $found_user_id && $connection->is_user_connected( $found_user_id ) ) { $new_master = $found_user_id; break; } diff --git a/tests/php/bootstrap.php b/tests/php/bootstrap.php index bc4b13aaf7dcd..f450f7cf46f81 100644 --- a/tests/php/bootstrap.php +++ b/tests/php/bootstrap.php @@ -71,6 +71,7 @@ function _manually_load_plugin() { require JETPACK_WOOCOMMERCE_INSTALL_DIR . '/woocommerce.php'; } require dirname( __FILE__ ) . '/../../jetpack.php'; + Jetpack::configure(); } function _manually_install_woocommerce() { diff --git a/tests/php/sync/class.silent-upgrader-skin.php b/tests/php/sync/class.silent-upgrader-skin.php index 99c57d2fa117e..22b554373cb79 100644 --- a/tests/php/sync/class.silent-upgrader-skin.php +++ b/tests/php/sync/class.silent-upgrader-skin.php @@ -1,5 +1,7 @@ assertEquals( '123', $this->server_replica_storage->get_option( 'foo_option_bar' ) ); } - public function test_sync_initalize_Jetpack_Sync_Action_on_init() { - // prioroty should be set so that plugins can set their own filers initialize the whitelist_filter before. - // Priority is set earlier now plugins_loaded but we plugins should still be able to set whitelist_filters by - // using the plugins_loaded action. - - $this->assertEquals( 90, has_action( 'plugins_loaded', array( 'Automattic\\Jetpack\\Sync\\Actions', 'init' ) ) ); - } - public function test_sync_default_options() { $this->setSyncClientDefaults(); // check that these values exists in the whitelist options