From b53fc512bfc247adf994f796130703aebf228790 Mon Sep 17 00:00:00 2001 From: Nestor Soriano Date: Mon, 22 Jun 2020 11:41:11 +0200 Subject: [PATCH] Some changes on the dependency injection engine based on feedback: - `wc_get_container` declares a return type now. - The autoloader now runs before `autoload_packages.php` is included. - The autloader now excludes the namespaces for Admin, Blocs, Rest API. - `private $container` in `Container` is now declared as being of type `\League\Container\Container`. - The underlying container is registered with `share` instead of `add`. - All methods in `AbstractServiceProvider` now allow passing a concrete. - `AbstractServiceProvider::addWithAutoArguments` now checks if an argument has a default value, and registers the argument so that that value is supplied when constructing the object. - `Proxies` class renamed to `ProxiesServiceProvider`. - The methods to call code in `LegacyProxy` now use `call_user_func_array`. --- src/Autoloader.php | 25 +++++++++++---- src/Container.php | 8 ++--- .../AbstractServiceProvider.php | 32 +++++++++++++------ ...Proxies.php => ProxiesServiceProvider.php} | 2 +- src/Tools/Proxies/LegacyProxy.php | 4 +-- woocommerce.php | 2 +- 6 files changed, 48 insertions(+), 25 deletions(-) rename src/Tools/DependencyManagement/ServiceProviders/{Proxies.php => ProxiesServiceProvider.php} (92%) diff --git a/src/Autoloader.php b/src/Autoloader.php index 97bed6315b7..7e138d24fb1 100644 --- a/src/Autoloader.php +++ b/src/Autoloader.php @@ -7,8 +7,6 @@ namespace Automattic\WooCommerce; -use PHPUnit\Exception; - defined( 'ABSPATH' ) || exit; /** @@ -23,6 +21,12 @@ class Autoloader { */ private function __construct() {} + const NON_CORE_WOO_NAMESPACES = array( + 'Automattic\\WooCommerce\\Admin\\', + 'Automattic\\WooCommerce\\Blocks\\', + 'Automattic\\WooCommerce\\RestApi\\', + ); + /** * Require the autoloader and return the result. * @@ -38,23 +42,32 @@ class Autoloader { return false; } + self::registerPsr4Autoloader(); + $autoloader_result = require $autoloader; if ( ! $autoloader_result ) { return false; } - self::registerPsr4Autoloader(); - return $autoloader_result; } /** * Define a PSR4 autoloader for the dependency injection engine to work. * Function grabbed from https://container.thephpleague.com/3.x + * + * TODO: Assess if this is still needed after https://github.com/Automattic/jetpack/pull/15106 is merged. + * If it still is, remove this notice. If it isn't, remove the method. */ protected static function registerPsr4Autoloader() { spl_autoload_register( function ( $class ) { + foreach ( self::NON_CORE_WOO_NAMESPACES as $non_core_namespace ) { + if ( $non_core_namespace === substr( $class, 0, strlen( $non_core_namespace ) ) ) { + return; + } + } + $prefix = 'Automattic\\WooCommerce\\'; $base_dir = __DIR__ . '/'; $len = strlen( $prefix ); @@ -64,9 +77,7 @@ class Autoloader { } $relative_class = substr( $class, $len ); $file = $base_dir . str_replace( '\\', '/', $relative_class ) . '.php'; - if ( file_exists( $file ) ) { - require $file; - } + require $file; } ); } diff --git a/src/Container.php b/src/Container.php index 960801fb225..28155e11fd5 100644 --- a/src/Container.php +++ b/src/Container.php @@ -7,7 +7,7 @@ namespace Automattic\WooCommerce; -use Automattic\WooCommerce\Tools\DependencyManagement\ServiceProviders as ServiceProviders; +use Automattic\WooCommerce\Tools\DependencyManagement\ServiceProviders\ProxiesServiceProvider; /** * PSR11 compliant dependency injection container for WooCommerce. @@ -34,13 +34,13 @@ final class Container implements \Psr\Container\ContainerInterface { * @var string[] */ private $service_providers = array( - ServiceProviders\Proxies::class, + ProxiesServiceProvider::class, ); /** * The underlying container. * - * @var \Psr\Container\ContainerInterface + * @var \League\Container\Container */ private $container; @@ -53,7 +53,7 @@ final class Container implements \Psr\Container\ContainerInterface { // Add ourselves as the shared instance of ContainerInterface, // register everything else using service providers. - $this->container->add( \Psr\Container\ContainerInterface::class, $this ); + $this->container->share( \Psr\Container\ContainerInterface::class, $this ); foreach ( $this->service_providers as $service_provider_class ) { $this->container->addServiceProvider( $service_provider_class ); diff --git a/src/Tools/DependencyManagement/AbstractServiceProvider.php b/src/Tools/DependencyManagement/AbstractServiceProvider.php index 57970941982..a14bdf0b19a 100644 --- a/src/Tools/DependencyManagement/AbstractServiceProvider.php +++ b/src/Tools/DependencyManagement/AbstractServiceProvider.php @@ -7,6 +7,7 @@ namespace Automattic\WooCommerce\Tools\DependencyManagement; +use League\Container\Argument\RawArgument; use League\Container\Definition\DefinitionInterface; use League\Container\Definition\Definition; @@ -27,21 +28,24 @@ abstract class AbstractServiceProvider extends \League\Container\ServiceProvider /** * Register a class in the container and use reflection to guess the constructor arguments. * + * WARNING: this method uses reflection, so please have performance in mind when using it. + * * @param string $class_name Class name to register. + * @param mixed $concrete The concrete to register. Can be a shared instance, a factory callback, or a class name. * @param bool $shared Whether to register the class as shared (`get` always returns the same instance) or not. * * @return DefinitionInterface The generated container definition. * * @throws \Exception Error when reflecting the class, or class constructor is not public, or an argument has no valid type hint. */ - public function addWithAutoArguments( string $class_name, bool $shared = false ) : DefinitionInterface { + public function addWithAutoArguments( string $class_name, $concrete = null, bool $shared = false ) : DefinitionInterface { try { $reflector = new \ReflectionClass( $class_name ); } catch ( \ReflectionException $ex ) { throw new \Exception( get_class( $this ) . "::addWithAutoArguments: error when reflecting class '$class_name': {$ex->getMessage()}" ); } - $definition = new Definition( $class_name, null ); + $definition = new Definition( $class_name, $concrete ); $constructor = $reflector->getConstructor(); @@ -52,12 +56,17 @@ abstract class AbstractServiceProvider extends \League\Container\ServiceProvider $constructor_arguments = $constructor->getParameters(); foreach ( $constructor_arguments as $argument ) { - $argument_class = $argument->getClass(); - if ( is_null( $argument_class ) ) { - throw new \Exception( get_class( $this ) . "::addWithAutoArguments: constructor argument '{$argument->getName()}' of class '$class_name' doesn't have a type hint or has one that doesn't specify a class." ); - } + if ( $argument->isDefaultValueAvailable() ) { + $default_value = $argument->getDefaultValue(); + $definition->addArgument( new RawArgument( $default_value ) ); + } else { + $argument_class = $argument->getClass(); + if ( is_null( $argument_class ) ) { + throw new \Exception( get_class( $this ) . "::addWithAutoArguments: constructor argument '{$argument->getName()}' of class '$class_name' doesn't have a type hint or has one that doesn't specify a class." ); + } - $definition->addArgument( $argument_class->name ); + $definition->addArgument( $argument_class->name ); + } } } @@ -72,14 +81,17 @@ abstract class AbstractServiceProvider extends \League\Container\ServiceProvider * Register a class in the container and use reflection to guess the constructor arguments. * The class is registered as shared, so `get` on the container always returns the same instance. * + * WARNING: this method uses reflection, so please have performance in mind when using it. + * * @param string $class_name Class name to register. + * @param mixed $concrete The concrete to register. Can be a shared instance, a factory callback, or a class name. * * @return DefinitionInterface The generated container definition. * * @throws \Exception Error when reflecting the class, or class constructor is not public, or an argument has no valid type hint. */ - public function shareWithAutoArguments( string $class_name ) : DefinitionInterface { - return $this->addWithAutoArguments( $class_name, true ); + public function shareWithAutoArguments( string $class_name, $concrete = null ) : DefinitionInterface { + return $this->addWithAutoArguments( $class_name, $concrete, true ); } /** @@ -104,6 +116,6 @@ abstract class AbstractServiceProvider extends \League\Container\ServiceProvider * @return DefinitionInterface The generated container definition. */ public function share( string $id, $concrete = null ) : DefinitionInterface { - return $this->getContainer()->share( $id, $concrete ); + return $this->add( $id, $concrete, true ); } } diff --git a/src/Tools/DependencyManagement/ServiceProviders/Proxies.php b/src/Tools/DependencyManagement/ServiceProviders/ProxiesServiceProvider.php similarity index 92% rename from src/Tools/DependencyManagement/ServiceProviders/Proxies.php rename to src/Tools/DependencyManagement/ServiceProviders/ProxiesServiceProvider.php index fb3b350ca70..4bfd847cb14 100644 --- a/src/Tools/DependencyManagement/ServiceProviders/Proxies.php +++ b/src/Tools/DependencyManagement/ServiceProviders/ProxiesServiceProvider.php @@ -13,7 +13,7 @@ use Automattic\WooCommerce\Tools\Proxies as ProxyClasses; /** * Service provider for the classes in the Automattic\WooCommerce\Tools\Proxies namespace. */ -class Proxies extends AbstractServiceProvider { +class ProxiesServiceProvider extends AbstractServiceProvider { /** * The classes/interfaces that are serviced by this service provider. diff --git a/src/Tools/Proxies/LegacyProxy.php b/src/Tools/Proxies/LegacyProxy.php index 8375dce002e..8d28b68a7a3 100644 --- a/src/Tools/Proxies/LegacyProxy.php +++ b/src/Tools/Proxies/LegacyProxy.php @@ -96,7 +96,7 @@ class LegacyProxy { * @return mixed The result from the function. */ public function callFunction( $function_name, ...$parameters ) { - return call_user_func( $function_name, ...$parameters ); + return call_user_func_array( $function_name, $parameters ); } /** @@ -110,6 +110,6 @@ class LegacyProxy { * @return mixed The result from the method. */ public function callStatic( $class_name, $method_name, ...$parameters ) { - return $class_name::$method_name( ...$parameters ); + return call_user_func_array( "$class_name::$method_name", $parameters ); } } diff --git a/woocommerce.php b/woocommerce.php index eed1db9dd4f..11256fc4494 100644 --- a/woocommerce.php +++ b/woocommerce.php @@ -53,7 +53,7 @@ function WC() { // phpcs:ignore WordPress.NamingConventions.ValidFunctionName.Fu * * @return \Psr\Container\ContainerInterface The WooCommerce PSR11 container. */ -function wc_get_container() { +function wc_get_container() : \Psr\Container\ContainerInterface { return $GLOBALS['wc_container']; }