Replaced constructor injection with method injection

Since we need to maintain backwards compatibility for class constructors we should settle on using method injection instead of constructor injection. I've replaced the `Definition` class we're using with one that doesn't support constructor arguments and added a check for auto_arg addition. Note that we don't check for method existence in the extended container. This is because reflection is unnecessarily expensive and we should avoid it if at all possible.
This commit is contained in:
Christopher Allford 2020-08-04 20:37:28 -07:00
parent 767fb048fc
commit d178c7ff01
18 changed files with 1008 additions and 1226 deletions

58
composer.lock generated
View File

@ -4,20 +4,20 @@
"Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies",
"This file is @generated automatically"
],
"content-hash": "877625af18978cccd2d02780fbd11842",
"content-hash": "286e909c9d9b2f4ea6b43b585e05b7cd",
"packages": [
{
"name": "automattic/jetpack-autoloader",
"version": "v2.0.2",
"version": "v2.1.0",
"source": {
"type": "git",
"url": "https://github.com/Automattic/jetpack-autoloader.git",
"reference": "4502da4b2443fc1b61389cacc94c34876aca2b3d"
"reference": "802517b3ff3010de89141d9f7c4d56aec1d21527"
},
"dist": {
"type": "zip",
"url": "https://api.github.com/repos/Automattic/jetpack-autoloader/zipball/4502da4b2443fc1b61389cacc94c34876aca2b3d",
"reference": "4502da4b2443fc1b61389cacc94c34876aca2b3d",
"url": "https://api.github.com/repos/Automattic/jetpack-autoloader/zipball/802517b3ff3010de89141d9f7c4d56aec1d21527",
"reference": "802517b3ff3010de89141d9f7c4d56aec1d21527",
"shasum": ""
},
"require": {
@ -40,20 +40,20 @@
"GPL-2.0-or-later"
],
"description": "Creates a custom autoloader for a plugin or theme.",
"time": "2020-07-09T13:18:38+00:00"
"time": "2020-07-27T20:37:00+00:00"
},
{
"name": "automattic/jetpack-constants",
"version": "v1.3.0",
"version": "v1.4.0",
"source": {
"type": "git",
"url": "https://github.com/Automattic/jetpack-constants.git",
"reference": "77e4a85601c63dc98b3dd282090eb8558a61685c"
"reference": "b4210d56948529b43785ce31e0055f435eac1f9f"
},
"dist": {
"type": "zip",
"url": "https://api.github.com/repos/Automattic/jetpack-constants/zipball/77e4a85601c63dc98b3dd282090eb8558a61685c",
"reference": "77e4a85601c63dc98b3dd282090eb8558a61685c",
"url": "https://api.github.com/repos/Automattic/jetpack-constants/zipball/b4210d56948529b43785ce31e0055f435eac1f9f",
"reference": "b4210d56948529b43785ce31e0055f435eac1f9f",
"shasum": ""
},
"require-dev": {
@ -71,7 +71,7 @@
"GPL-2.0-or-later"
],
"description": "A wrapper for defining constants in a more testable way.",
"time": "2020-06-22T11:37:41+00:00"
"time": "2020-07-01T15:55:35+00:00"
},
{
"name": "composer/installers",
@ -597,7 +597,7 @@
],
"description": "A modern, javascript-driven WooCommerce Admin experience.",
"homepage": "https://github.com/woocommerce/woocommerce-admin",
"time": "2020-07-28T15:18:55+00:00"
"time": "2020-07-28T00:28:40+00:00"
},
{
"name": "woocommerce/woocommerce-blocks",
@ -808,6 +808,20 @@
"constructor",
"instantiate"
],
"funding": [
{
"url": "https://www.doctrine-project.org/sponsorship.html",
"type": "custom"
},
{
"url": "https://www.patreon.com/phpdoctrine",
"type": "patreon"
},
{
"url": "https://tidelift.com/funding/github/packagist/doctrine%2Finstantiator",
"type": "tidelift"
}
],
"time": "2020-05-29T17:27:14+00:00"
},
{
@ -1070,6 +1084,12 @@
"object",
"object graph"
],
"funding": [
{
"url": "https://tidelift.com/funding/github/packagist/myclabs/deep-copy",
"type": "tidelift"
}
],
"time": "2020-06-29T13:22:24+00:00"
},
{
@ -2670,6 +2690,20 @@
"polyfill",
"portable"
],
"funding": [
{
"url": "https://symfony.com/sponsor",
"type": "custom"
},
{
"url": "https://github.com/fabpot",
"type": "github"
},
{
"url": "https://tidelift.com/funding/github/packagist/symfony/symfony",
"type": "tidelift"
}
],
"time": "2020-07-14T12:35:20+00:00"
},
{

1796
package-lock.json generated

File diff suppressed because it is too large Load Diff

View File

@ -29,11 +29,6 @@ use Automattic\WooCommerce\Internal\DependencyManagement\ExtendedContainer;
*/
final class Container implements \Psr\Container\ContainerInterface {
/**
* The root namespace of all WooCommerce classes in the `src` directory.
*/
const WOOCOMMERCE_ROOT_NAMESPACE = 'Automattic\\WooCommerce';
/**
* The list of service provider classes to register.
*

View File

@ -9,7 +9,6 @@ namespace Automattic\WooCommerce\Internal\DependencyManagement;
use League\Container\Argument\RawArgument;
use League\Container\Definition\DefinitionInterface;
use League\Container\Definition\Definition;
/**
* Base class for the service providers used to register classes in the container.
@ -50,7 +49,7 @@ abstract class AbstractServiceProvider extends \League\Container\ServiceProvider
} else {
$argument_class = $argument->getClass();
if ( is_null( $argument_class ) ) {
throw new ContainerException( "AbstractServiceProvider::add_with_auto_arguments: constructor argument '{$argument->getName()}' of class '$class_name' doesn't have a type hint or has one that doesn't specify a class." );
throw new ContainerException( "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 );
@ -59,7 +58,6 @@ abstract class AbstractServiceProvider extends \League\Container\ServiceProvider
}
// Register the definition only after being sure that no exception will be thrown.
$this->getContainer()->add( $definition->getAlias(), $definition, $shared );
return $definition;
@ -78,21 +76,20 @@ abstract class AbstractServiceProvider extends \League\Container\ServiceProvider
private function reflect_class_or_callable( string $class_name, $concrete ) {
if ( ! isset( $concrete ) || is_string( $concrete ) && class_exists( $concrete ) ) {
try {
$class = $concrete ?? $class_name;
$reflector = new \ReflectionClass( $class );
$constructor = $reflector->getConstructor();
if ( isset( $constructor ) && ! $constructor->isPublic() ) {
throw new ContainerException( "AbstractServiceProvider::add_with_auto_arguments: constructor of class '$class' isn't public, instances can't be created." );
$class = $concrete ?? $class_name;
$method = new \ReflectionMethod( $class, Definition::INJECTION_METHOD );
if ( isset( $method ) && ! $method->isPublic() ) {
throw new ContainerException( "Method '" . Definition::INJECTION_METHOD . "' of class '$class' isn't public, instances can't be created." );
}
return $constructor;
return $method;
} catch ( \ReflectionException $ex ) {
throw new ContainerException( "AbstractServiceProvider::add_with_auto_arguments: error when reflecting class '$class': {$ex->getMessage()}" );
return null;
}
} elseif ( is_callable( $concrete ) ) {
try {
return new \ReflectionFunction( $concrete );
} catch ( \ReflectionException $ex ) {
throw new ContainerException( "AbstractServiceProvider::add_with_auto_arguments: error when reflecting callable: {$ex->getMessage()}" );
throw new ContainerException( "Error when reflecting callable: {$ex->getMessage()}" );
}
}

View File

@ -0,0 +1,41 @@
<?php
/**
* An extension to the Definition class to prevent constructor injection from being possible.
*
* @package Automattic\WooCommerce\Internal\DependencyManagement
*/
namespace Automattic\WooCommerce\Internal\DependencyManagement;
use \League\Container\Definition\Definition as BaseDefinition;
/**
* An extension of the definition class that replaces constructor injection with method injection.
*/
class Definition extends BaseDefinition {
/**
* The standard method that we use for dependency injection.
*/
const INJECTION_METHOD = 'set_internal_dependencies';
/**
* Resolve a class using method injection instead of constructor injection.
*
* @param string $concrete The concrete to instantiate.
*
* @return object
*/
protected function resolveClass( string $concrete ) {
$resolved = $this->resolveArguments( $this->arguments );
$concrete = new $concrete();
// Constructor injection causes backwards compatibility problems
// so we will rely on method injection via an internal method.
if ( method_exists( $concrete, static::INJECTION_METHOD ) ) {
call_user_func_array( array( $concrete, static::INJECTION_METHOD ), $resolved );
}
return $concrete;
}
}

View File

@ -7,14 +7,22 @@
namespace Automattic\WooCommerce\Internal\DependencyManagement;
use Automattic\WooCommerce\Container;
use Automattic\WooCommerce\Utilities\StringUtility;
use League\Container\Container as BaseContainer;
use League\Container\Definition\DefinitionInterface;
/**
* This class extends the original League's Container object by adding some functionality
* that we need for WooCommerce.
*/
class ExtendedContainer extends \League\Container\Container {
class ExtendedContainer extends BaseContainer {
/**
* The root namespace of all WooCommerce classes in the `src` directory.
*
* @var string
*/
private $woocommerce_namespace = 'Automattic\\WooCommerce\\';
/**
* Whitelist of classes that we can register using the container
@ -43,24 +51,23 @@ class ExtendedContainer extends \League\Container\Container {
* @throws ContainerException Invalid parameters.
*/
public function add( string $class_name, $concrete = null, bool $shared = null ) : DefinitionInterface {
if ( ! $this->class_is_in_root_namespace( $class_name ) && ! in_array( $class_name, $this->registration_whitelist, true ) ) {
throw new ContainerException( "Can't use the container to register '$class_name', only objects in the " . Container::WOOCOMMERCE_ROOT_NAMESPACE . ' namespace are allowed for registration.' );
if ( ! $this->is_class_allowed( $class_name ) ) {
throw new ContainerException( "You cannot add '$class_name', only classes in the {$this->woocommerce_namespace} namespace are allowed." );
}
$concrete_class = $this->get_class_from_concrete( $concrete );
if ( isset( $concrete_class ) && ! $this->is_class_allowed( $concrete_class ) ) {
throw new ContainerException( "You cannot add concrete '$concrete_class', only classes in the {$this->woocommerce_namespace} namespace are allowed." );
}
// We want to use a definition class that does not support constructor injection to avoid accidental usage.
if ( ! $concrete instanceof DefinitionInterface ) {
$concrete = new Definition( $class_name, $concrete );
}
return parent::add( $class_name, $concrete, $shared );
}
/**
* Does a class belong to the WooCommerce root namespace?
*
* @param string $class_name The class name to check.
*
* @return bool True if the class belongs to the WooCommerce root namespace.
*/
private function class_is_in_root_namespace( $class_name ) {
return substr( $class_name, 0, strlen( Container::WOOCOMMERCE_ROOT_NAMESPACE ) + 1 ) === Container::WOOCOMMERCE_ROOT_NAMESPACE . '\\';
}
/**
* Replace an existing registration with a different concrete.
*
@ -70,9 +77,14 @@ class ExtendedContainer extends \League\Container\Container {
* @return DefinitionInterface The modified definition.
* @throws ContainerException Invalid parameters.
*/
public function replace( string $class_name, $concrete ) {
public function replace( string $class_name, $concrete ) : DefinitionInterface {
if ( ! $this->has( $class_name ) ) {
throw new ContainerException( "ExtendedContainer::replace: The container doesn't have '$class_name' registered, please use 'add' instead of 'replace'." );
throw new ContainerException( "The container doesn't have '$class_name' registered, please use 'add' instead of 'replace'." );
}
$concrete_class = $this->get_class_from_concrete( $concrete );
if ( isset( $concrete_class ) && ! $this->is_class_allowed( $concrete_class ) ) {
throw new ContainerException( "You cannot use concrete '$concrete_class', only classes in the {$this->woocommerce_namespace} namespace are allowed." );
}
return $this->extend( $class_name )->setConcrete( $concrete );
@ -105,4 +117,38 @@ class ExtendedContainer extends \League\Container\Container {
return parent::get( $id, $new );
}
/**
* Gets the class from the concrete regardless of type.
*
* @param mixed $concrete The concrete that we want the class from..
*
* @return string|null The class from the concrete if one is available, null otherwise.
*/
protected function get_class_from_concrete( $concrete ) {
if ( is_object( $concrete ) && ! is_callable( $concrete ) ) {
if ( $concrete instanceof DefinitionInterface ) {
return $this->get_class_from_concrete( $concrete->getConcrete() );
}
return get_class( $concrete );
}
if ( is_string( $concrete ) && class_exists( $concrete ) ) {
return $concrete;
}
return null;
}
/**
* Checks to see whether or not a class is allowed to be registered.
*
* @param string $class_name The class to check.
*
* @return bool True if the class is allowed to be registered, false otherwise.
*/
protected function is_class_allowed( string $class_name ): bool {
return StringUtility::starts_with( $class_name, $this->woocommerce_namespace, false ) || in_array( $class_name, $this->registration_whitelist, true );
}
}

View File

@ -38,7 +38,7 @@ class LegacyProxy {
*/
public function get_instance_of( string $class_name, ...$args ) {
if ( false !== strpos( $class_name, '\\' ) ) {
throw new \Exception( 'The LegacyProxy class is not intended for getting instances of classes in the src directory, please use constructor injection or the instance of \\Psr\\Container\\ContainerInterface for that.' );
throw new \Exception( 'The LegacyProxy class is not intended for getting instances of classes in the src directory, please use method injection or the instance of \\Psr\\Container\\ContainerInterface for that.' );
}
// If a class has a dedicated method to obtain a instance, use it.

View File

@ -69,7 +69,7 @@ _Resolving_ a class means asking the container to provide an instance of the cla
In principle, the container should be used to register and resolve all the classes in the `src` directory. The exception might be data-only classes that could be created the old way (using a plain `new` statement); but as a rule of thumb, the container should always be used.
There are two ways to resolve registered classes, depending on from where they are resolved:
* Classes in the `src` directory specify their dependencies as constructor arguments, which are automatically supplied by the container when the class is resolved (this is called _constructor injection_).
* Classes in the `src` directory specify their dependencies as `set_internal_dependencies` arguments, which are automatically supplied by the container when the class is resolved (this is called _dependency injection_).
* For code in the `includes` directory there's a `wc_get_container` function that will return the container, then its `get` method can be used to resolve any class.
### Resolving classes
@ -78,7 +78,7 @@ There are two ways to resolve registered classes, depending on from where they n
#### 1. Other classes in the `src` directory
When a class in the `src` directory depends on other one classes from the same directory, it should use constructor injection. This means specifying these dependencies as constructor arguments with appropriate type hints, and storing these in private variables, ready to be used when needed:
When a class in the `src` directory depends on other one classes from the same directory, it should use method injection. This means specifying these dependencies as arguments in a `set_internal_dependencies` method with appropriate type hints, and storing these in private variables, ready to be used when needed:
```php
use TheService1Namespace\Service1;
@ -89,7 +89,7 @@ class TheClassWithDependencies {
private $service2;
public function __construct( Service1Class $service1, Service2Class $service2 ) {
public function set_internal_dependencies( Service1Class $service1, Service2Class $service2 ) {
$this->$service1 = $service1;
$this->$service2 = $service2;
}
@ -100,9 +100,9 @@ class TheClassWithDependencies {
}
```
Whenever the container is about to resolve `TheClassWithDependencies` it will also resolve `Service1Class` and `Service2Class` and pass them as constructor arguments to the requested class. If these service classes have constructor arguments too then those will also be appropriately resolved recursively.
Whenever the container is about to resolve `TheClassWithDependencies` it will also resolve `Service1Class` and `Service2Class` and pass them as method arguments to the requested class. If these service classes have method arguments too then those will also be appropriately resolved recursively.
A "lazy" approach is also possible if needed: you can specify the container itself as a constructor argument (using `\Psr\Container\ContainerInterface` as type hint), and use its `get` method to obtain the required instance at the appropriate time:
A "lazy" approach is also possible if needed: you can specify the container itself as a method argument (using `\Psr\Container\ContainerInterface` as type hint), and use its `get` method to obtain the required instance at the appropriate time:
```php
use TheService1Namespace\Service1;
@ -110,7 +110,7 @@ use TheService1Namespace\Service1;
class TheClassWithDependencies {
private $container;
public function __construct( \Psr\Container\ContainerInterface $container ) {
public function set_internal_dependencies( \Psr\Container\ContainerInterface $container ) {
$this->$container = $container;
}
@ -120,7 +120,7 @@ class TheClassWithDependencies {
}
```
In general, however, constructor injection is preferred and the lazy approach should be used only when really necessary.
In general, however, method injection is strongly preferred and the lazy approach should be used only when really necessary.
#### 2. Code in the `includes` directory
@ -146,7 +146,7 @@ For a class to be resolvable using the container, it needs to have been previous
The `Container` class is "read-only", in that it has a `get` method to resolve classes but it doesn't have any method to register classes. Instead, class registration is done by using [service providers](https://container.thephpleague.com/3.x/service-providers/). That's how the whole process would go when creating a new class:
First, create the class in the appropriate namespace (and thus in the matching folder), remember that the base namespace for the classes in the `src` directory is `Atuomattic\WooCommerce`. If the class depends on other classes from `src`, specify these dependencies as constructor arguments in detailed above.
First, create the class in the appropriate namespace (and thus in the matching folder), remember that the base namespace for the classes in the `src` directory is `Atuomattic\WooCommerce`. If the class depends on other classes from `src`, specify these dependencies as `set_internal_dependencies` arguments in detailed above.
Example of such a class:
@ -158,7 +158,7 @@ use Automattic\WooCommerce\TheDependencyNamespace\TheDependencyClass;
class TheClass {
private $the_dependency;
public function __construct( TheDependencyClass $dependency ) {
public function set_internal_dependencies( TheDependencyClass $dependency ) {
$this->the_dependency = $dependency;
}
@ -195,7 +195,7 @@ Worth noting:
* If you look at [the service provider documentation](https://container.thephpleague.com/3.x/service-providers/) you will see that classes are registered using `this->getContainer()->add`. WooCommerce's `AbstractServiceProvider` adds a utility `add` method itself that serves the same purpose.
* You can use `share` instead of `add` to register single-instance classes (the class is instantiated only once and cached, so the same instance is returned every time the class is resolved).
If the class being registered has constructor arguments then the `add` (or `share`) method must be followed by as many `addArguments` calls as needed. WooCommerce's `AbstractServiceProvider` adds a utility `add_with_auto_arguments` method (and a sibling `share_with_auto_arguments` method) that uses reflection to figure out and register all the constructor arguments (which need to have type hints). Please have in mind the possible performance penalty incurred by the usage of reflection when using this helper method.
If the class being registered has `set_internal_dependencies` arguments then the `add` (or `share`) method must be followed by as many `addArguments` calls as needed. WooCommerce's `AbstractServiceProvider` adds a utility `add_with_auto_arguments` method (and a sibling `share_with_auto_arguments` method) that uses reflection to figure out and register all the `set_internal_dependencies` arguments (which need to have type hints). Please have in mind the possible performance penalty incurred by the usage of reflection when using this helper method.
An alternative version of the service provider, which is used to register both the class and its dependency, and which takes advantage of `add_with_auto_arguments`, could be as follows:
@ -259,7 +259,7 @@ Note that if the closure is defined as a function with arguments, the supplied p
The container is intended for registering **only** classes in the `src` folder. There is a check in place to prevent classes outside the root `Automattic\Woocommerce` namespace from being registered.
This implies that classes outside `src` can't be constructor-injected, and thus must not be used as type hints in constructor arguments. There are mechanisms in place to interact with "outside" code (including code from the `includes` folder and third-party code) in a way that makes it easy to write unit tests.
This implies that classes outside `src` can't be dependency-injected, and thus must not be used as type hints in `set_internal_dependencies` arguments. There are mechanisms in place to interact with "outside" code (including code from the `includes` folder and third-party code) in a way that makes it easy to write unit tests.
## The `Internal` namespace
@ -298,7 +298,7 @@ But how does using `LegacyProxy` help in making the code testable? The trick is
### Using the legacy proxy
`LegacyProxy` is a class that is registered in the container as any other class, so an instance can be obtained by using constructor injection:
`LegacyProxy` is a class that is registered in the container as any other class, so an instance can be obtained by using dependency-injection:
```php
use Automattic\WooCommerce\Proxies\LegacyProxy;
@ -306,7 +306,7 @@ use Automattic\WooCommerce\Proxies\LegacyProxy;
class TheClass {
private $legacy_proxy;
public function __construct( LegacyProxy $legacy_proxy ) {
public function set_internal_dependencies( LegacyProxy $legacy_proxy ) {
$this->legacy_proxy = $legacy_proxy;
}
@ -316,7 +316,7 @@ class TheClass {
}
```
However, the recommended way (especially when no other dependencies need to be constructor-injected) is to use the equivalent methods in the `WooCommerce` class via the `WC()` helper, like this:
However, the recommended way (especially when no other dependencies need to be dependency-injected) is to use the equivalent methods in the `WooCommerce` class via the `WC()` helper, like this:
```php
class TheClass {
@ -382,14 +382,14 @@ class ActionsProxy {
}
```
Note however that such a class would have to be explicitly constructor-injected (unless additional helper methods are defined in the `WooCommerce` class), and that you would need to create a pairing mock class (e.g. `MockableActionsProxy`) and replace the original registration using `wc_get_container()->replace( ActionsProxy::class, MockableActionsProxy::class )`.
Note however that such a class would have to be explicitly dependency-injected (unless additional helper methods are defined in the `WooCommerce` class), and that you would need to create a pairing mock class (e.g. `MockableActionsProxy`) and replace the original registration using `wc_get_container()->replace( ActionsProxy::class, MockableActionsProxy::class )`.
## Defining new actions and filters
WordPress' hooks (actions and filters) are a very powerful extensibility mechanism and it's the core tool that allows WooCommerce extensions to be developer. However it has been often (ab)used in the WooCommerce core codebase to drive internal logic, e.g. an action is triggered from within one class or function with the assumption that somewhere there's some other class or function that will handle it and continue whatever processing is supposed to happen.
In order to keep the code as easy as reasonably possible to read and maintain, **hooks shouldn't be used to drive WooCommerce's internal logic and processes**. If you need the services of a given class or function, please call these directly (by using constructor injection or the legacy proxy as appropriate to get access to the desired service). **New hooks should be introduced only if they provide a valuable extension point for plugins**.
In order to keep the code as easy as reasonably possible to read and maintain, **hooks shouldn't be used to drive WooCommerce's internal logic and processes**. If you need the services of a given class or function, please call these directly (by using dependency-injection or the legacy proxy as appropriate to get access to the desired service). **New hooks should be introduced only if they provide a valuable extension point for plugins**.
As usual, there might be reasonable exceptions to this; but please keep this rule in mind whenever you consider creating a new hook.
@ -400,11 +400,11 @@ Unit tests are a fundamental tool to keep the code reliable and reasonably safe
**If you are a WooCommerce core team member or a contributor from other team at Automattic:** Please write unit tests to cover any code addition or modification that you make to the `src` directory (and ideally the same for the `includes` directory, by the way). There are always reasonable exceptions, but the rule of thumb is that all code should be covered by tests.
**If you are an external contributor:** When adding or changing code on the WooCommerce codebase, and especially in the `src` directory, adding unit tests is recommended but not mandatory: no contributions will be rejected solely for lacking unit tests. However, please try to at least make the code easily testable by honoring the container and constructor injection mechanism, and by using the legacy proxy to interact with legacy code when needed. If you do so, the WooCommerce team or other contributors will be able to add the missing tests.
**If you are an external contributor:** When adding or changing code on the WooCommerce codebase, and especially in the `src` directory, adding unit tests is recommended but not mandatory: no contributions will be rejected solely for lacking unit tests. However, please try to at least make the code easily testable by honoring the container and dependency-injection mechanism, and by using the legacy proxy to interact with legacy code when needed. If you do so, the WooCommerce team or other contributors will be able to add the missing tests.
### Mocking dependencies
Since all the dependencies for classes in this directory are constructor-injected or retrieved lazily by directly accessing the container, it's easy to mock them by either manually creating a mock class with the same public surface or by using [PHPUnit's test doubles](https://phpunit.readthedocs.io/en/9.2/test-doubles.html):
Since all the dependencies for classes in this directory are dependency-injected or retrieved lazily by directly accessing the container, it's easy to mock them by either manually creating a mock class with the same public surface or by using [PHPUnit's test doubles](https://phpunit.readthedocs.io/en/9.2/test-doubles.html):
```php
$dependency_mock = somehow_create_mock();

View File

@ -0,0 +1,62 @@
<?php
/**
* A class of utilities for dealing with namespaces.
*
* @package Automattic\WooCommerce\Utilities
*/
namespace Automattic\WooCommerce\Utilities;
/**
* A class of utilities for dealing with namespaces.
*/
final class StringUtility {
/**
* Checks to see whether or not a string starts with another.
*
* @param string $string The string we want to check.
* @param string $starts_with The string we're looking for at the start of $string.
* @param bool $case_sensitive Indicates whether the comparison should be case-sensitive.
*
* @return bool True if the $string starts with $starts_with, false otherwise.
*/
public static function starts_with( string $string, string $starts_with, bool $case_sensitive = true ): bool {
$len = strlen( $starts_with );
if ( $len > strlen( $string ) ) {
return false;
}
$string = substr( $string, 0, $len );
if ( $case_sensitive ) {
return strcmp( $string, $starts_with ) === 0;
}
return strcasecmp( $string, $starts_with ) === 0;
}
/**
* Checks to see whether or not a string ends with another.
*
* @param string $string The string we want to check.
* @param string $ends_with The string we're looking for at the end of $string.
* @param bool $case_sensitive Indicates whether the comparison should be case-sensitive.
*
* @return bool True if the $string ends with $ends_with, false otherwise.
*/
public static function ends_with( string $string, string $ends_with, bool $case_sensitive = true ): bool {
$len = strlen( $ends_with );
if ( $len > strlen( $string ) ) {
return false;
}
$string = substr( $string, -$len );
if ( $case_sensitive ) {
return strcmp( $string, $ends_with ) === 0;
}
return strcasecmp( $string, $ends_with ) === 0;
}
}

View File

@ -9,6 +9,7 @@ namespace Automattic\WooCommerce\Tests\Internal\DependencyManagement;
use Automattic\WooCommerce\Internal\DependencyManagement\AbstractServiceProvider;
use Automattic\WooCommerce\Internal\DependencyManagement\ContainerException;
use Automattic\WooCommerce\Internal\DependencyManagement\Definition;
use Automattic\WooCommerce\Internal\DependencyManagement\ExtendedContainer;
use Automattic\WooCommerce\Tests\Internal\DependencyManagement\ExampleClasses\ClassWithConstructorArgumentWithoutTypeHint;
use Automattic\WooCommerce\Tests\Internal\DependencyManagement\ExampleClasses\ClassWithDependencies;
@ -84,47 +85,47 @@ class AbstractServiceProviderTest extends \WC_Unit_Test_Case {
*/
public function test_add_with_auto_arguments_throws_on_non_class_passed_as_class_name() {
$this->expectException( ContainerException::class );
$this->expectExceptionMessage( "AbstractServiceProvider::add_with_auto_arguments: error when reflecting class 'foobar': Class foobar does not exist" );
$this->expectExceptionMessage( "You cannot add 'foobar', only classes in the Automattic\WooCommerce\ namespace are allowed." );
$this->sut->add_with_auto_arguments( 'foobar' );
}
/**
* @testdox 'add_with_auto_arguments' should throw an exception if the passed class has a private constructor.
* @testdox 'add_with_auto_arguments' should throw an exception if the passed class has a private method.
*/
public function test_add_with_auto_arguments_throws_on_class_private_constructor() {
public function test_add_with_auto_arguments_throws_on_class_private_method_injection() {
$this->expectException( ContainerException::class );
$this->expectExceptionMessage( "AbstractServiceProvider::add_with_auto_arguments: constructor of class '" . ClassWithPrivateConstructor::class . "' isn't public, instances can't be created." );
$this->expectExceptionMessage( "Method '" . Definition::INJECTION_METHOD . "' of class '" . ClassWithPrivateConstructor::class . "' isn't public, instances can't be created." );
$this->sut->add_with_auto_arguments( ClassWithPrivateConstructor::class );
}
/**
* @testdox 'add_with_auto_arguments' should throw an exception if the passed concrete is a class with a private constructor.
* @testdox 'add_with_auto_arguments' should throw an exception if the passed concrete is a class with a private method.
*/
public function test_add_with_auto_arguments_throws_on_concrete_private_constructor() {
public function test_add_with_auto_arguments_throws_on_concrete_private_method_injection() {
$this->expectException( ContainerException::class );
$this->expectExceptionMessage( "AbstractServiceProvider::add_with_auto_arguments: constructor of class '" . ClassWithPrivateConstructor::class . "' isn't public, instances can't be created." );
$this->expectExceptionMessage( "Method '" . Definition::INJECTION_METHOD . "' of class '" . ClassWithPrivateConstructor::class . "' isn't public, instances can't be created." );
$this->sut->add_with_auto_arguments( ClassWithDependencies::class, ClassWithPrivateConstructor::class );
}
/**
* @testdox 'add_with_auto_arguments' should throw an exception if the passed class has a constructor argument without type hint.
* @testdox 'add_with_auto_arguments' should throw an exception if the passed class has a method argument without type hint.
*/
public function test_add_with_auto_arguments_throws_on_constructor_argument_without_type_hint() {
public function test_add_with_auto_arguments_throws_on_method_argument_without_type_hint() {
$this->expectException( ContainerException::class );
$this->expectExceptionMessage( "AbstractServiceProvider::add_with_auto_arguments: constructor argument 'argument_without_type_hint' of class '" . ClassWithConstructorArgumentWithoutTypeHint::class . "' doesn't have a type hint or has one that doesn't specify a class." );
$this->expectExceptionMessage( "Argument 'argument_without_type_hint' of class '" . ClassWithConstructorArgumentWithoutTypeHint::class . "' doesn't have a type hint or has one that doesn't specify a class." );
$this->sut->add_with_auto_arguments( ClassWithConstructorArgumentWithoutTypeHint::class );
}
/**
* @testdox 'add_with_auto_arguments' should throw an exception if the passed class has a constructor argument with a scalar type hint.
* @testdox 'add_with_auto_arguments' should throw an exception if the passed class has a method argument with a scalar type hint.
*/
public function test_add_with_auto_arguments_throws_on_constructor_argument_with_scalar_type_hint() {
public function test_add_with_auto_arguments_throws_on_method_argument_with_scalar_type_hint() {
$this->expectException( ContainerException::class );
$this->expectExceptionMessage( "AbstractServiceProvider::add_with_auto_arguments: constructor argument 'scalar_argument_without_default_value' of class '" . ClassWithScalarConstructorArgument::class . "' doesn't have a type hint or has one that doesn't specify a class." );
$this->expectExceptionMessage( "Argument 'scalar_argument_without_default_value' of class '" . ClassWithScalarConstructorArgument::class . "' doesn't have a type hint or has one that doesn't specify a class." );
$this->sut->add_with_auto_arguments( ClassWithScalarConstructorArgument::class );
}

View File

@ -13,10 +13,10 @@ namespace Automattic\WooCommerce\Tests\Internal\DependencyManagement\ExampleClas
class ClassWithConstructorArgumentWithoutTypeHint {
/**
* Class constructor.
* Sets class dependencies.
*
* @param mixed $argument_without_type_hint Anything, really.
*/
public function __construct( $argument_without_type_hint ) {
public function set_internal_dependencies( $argument_without_type_hint ) {
}
}

View File

@ -39,14 +39,14 @@ class ClassWithDependencies {
public $dependency_class = null;
/**
* Class constructor.
* Sets the dependencies for the class.
*
* @param DependencyClass $dependency_class A class we depend on.
* @param int $some_number Some number we need for some reason.
*/
public function __construct( DependencyClass $dependency_class, int $some_number = self::SOME_NUMBER ) {
public function set_internal_dependencies( DependencyClass $dependency_class, int $some_number = self::SOME_NUMBER ) {
self::$instances_count++;
$this->dependency_class = $dependency_class;
$this->some_number = $some_number;
$this->some_number = self::SOME_NUMBER;
}
}

View File

@ -13,8 +13,8 @@ namespace Automattic\WooCommerce\Tests\Internal\DependencyManagement\ExampleClas
class ClassWithPrivateConstructor {
/**
* Class constructor.
* Sets class dependencies.
*/
private function __construct() {
private function set_internal_dependencies() {
}
}

View File

@ -15,10 +15,10 @@ class ClassWithScalarConstructorArgument {
// phpcs:disable Squiz.Commenting.FunctionComment.InvalidTypeHint
/**
* Class constructor.
* Sets class dependencies.
*
* @param mixed $scalar_argument_without_default_value Anything, really.
*/
public function __construct( int $scalar_argument_without_default_value ) {
public function set_internal_dependencies( int $scalar_argument_without_default_value ) {
}
}

View File

@ -38,11 +38,23 @@ class ExtendedContainerTest extends \WC_Unit_Test_Case {
$external_class = \League\Container\Container::class;
$this->expectException( ContainerException::class );
$this->expectExceptionMessage( "Can't use the container to register '" . $external_class . "', only objects in the Automattic\WooCommerce namespace are allowed for registration." );
$this->expectExceptionMessage( "You cannot add '$external_class', only classes in the Automattic\WooCommerce\ namespace are allowed." );
$this->sut->add( $external_class );
}
/**
* @testdox 'add' should throw an exception when trying to register a concrete class not in the WooCommerce root namespace.
*/
public function test_add_throws_when_trying_to_register_concrete_class_in_forbidden_namespace() {
$external_class = \League\Container\Container::class;
$this->expectException( ContainerException::class );
$this->expectExceptionMessage( "You cannot add concrete '$external_class', only classes in the Automattic\WooCommerce\ namespace are allowed." );
$this->sut->add( DependencyClass::class, $external_class );
}
/**
* @testdox 'add' should allow registering classes in the WooCommerce root namespace.
*/
@ -59,11 +71,26 @@ class ExtendedContainerTest extends \WC_Unit_Test_Case {
*/
public function test_replace_throws_if_class_has_not_been_registered() {
$this->expectException( ContainerException::class );
$this->expectExceptionMessage( "ExtendedContainer::replace: The container doesn't have '" . DependencyClass::class . "' registered, please use 'add' instead of 'replace'." );
$this->expectExceptionMessage( "The container doesn't have '" . DependencyClass::class . "' registered, please use 'add' instead of 'replace'." );
$this->sut->replace( DependencyClass::class, null );
}
/**
* @testdox 'replace'
*/
public function test_replace_throws_if_concrete_not_in_woocommerce_root_namespace() {
$instance = new DependencyClass();
$this->sut->add( DependencyClass::class, $instance, true );
$external_class = \League\Container\Container::class;
$this->expectException( ContainerException::class );
$this->expectExceptionMessage( "You cannot use concrete '$external_class', only classes in the Automattic\WooCommerce\ namespace are allowed." );
$this->sut->replace( DependencyClass::class, $external_class );
}
/**
* @testdox 'replace' should allow to replace existing registrations.
*/

View File

@ -10,7 +10,7 @@ namespace Automattic\WooCommerce\Tests\Proxies\ExampleClasses;
use Automattic\WooCommerce\Proxies\LegacyProxy;
/**
* An example class that uses the legacy proxy both from an constructor injected proxy and from the helper methods in the WooCommerce class.
* An example class that uses the legacy proxy both from a dependency injected proxy and from the helper methods in the WooCommerce class.
*/
class ClassThatDependsOnLegacyCode {
/**
@ -21,11 +21,11 @@ class ClassThatDependsOnLegacyCode {
private $legacy_proxy;
/**
* Class constructor.
* Sets class dependencies.
*
* @param LegacyProxy $legacy_proxy The instance of LegacyProxy to use.
*/
public function __construct( LegacyProxy $legacy_proxy ) {
public function set_internal_dependencies( LegacyProxy $legacy_proxy ) {
$this->legacy_proxy = $legacy_proxy;
}

View File

@ -33,7 +33,7 @@ class LegacyProxyTest extends \WC_Unit_Test_Case {
*/
public function test_get_instance_of_throws_when_trying_to_get_a_namespaced_class() {
$this->expectException( \Exception::class );
$this->expectExceptionMessage( 'The LegacyProxy class is not intended for getting instances of classes in the src directory, please use constructor injection or the instance of \Psr\Container\ContainerInterface for that.' );
$this->expectExceptionMessage( 'The LegacyProxy class is not intended for getting instances of classes in the src directory, please use method injection or the instance of \Psr\Container\ContainerInterface for that.' );
$this->sut->get_instance_of( DependencyClass::class );
}

View File

@ -0,0 +1,51 @@
<?php
namespace Automattic\WooCommerce\Tests\Utilities;
use Automattic\WooCommerce\Utilities\StringUtility;
/**
* A collection of tests for the string utility class.
*/
class StringUtilityTest extends \WC_Unit_Test_Case {
/**
* @testdox `starts_with` should check whether one string starts with another.
*/
public function test_starts_with() {
$this->assertTrue( StringUtility::starts_with( 'test', 'te' ) );
$this->assertTrue( StringUtility::starts_with( ' foo bar', ' foo' ) );
$this->assertFalse( StringUtility::starts_with( 'test', 'st' ) );
$this->assertFalse( StringUtility::starts_with( ' foo bar', ' bar' ) );
$this->assertTrue( StringUtility::starts_with( 'TEST', 'te', false ) );
$this->assertTrue( StringUtility::starts_with( ' FOO BAR', ' foo', false ) );
$this->assertFalse( StringUtility::starts_with( 'TEST', 'st', false ) );
$this->assertFalse( StringUtility::starts_with( ' FOO BAR', ' bar', false ) );
$this->assertTrue( StringUtility::starts_with( 'test', 'TE', false ) );
$this->assertTrue( StringUtility::starts_with( ' foo bar', ' FOO', false ) );
$this->assertFalse( StringUtility::starts_with( 'test', 'ST', false ) );
$this->assertFalse( StringUtility::starts_with( ' foo bar', ' BAR', false ) );
}
/**
* @testdox `ends_with` should check whether one string ends with another.
*/
public function test_ends_with() {
$this->assertFalse( StringUtility::ends_with( 'test', 'te' ) );
$this->assertFalse( StringUtility::ends_with( ' foo bar', ' foo' ) );
$this->assertTrue( StringUtility::ends_with( 'test', 'st' ) );
$this->assertTrue( StringUtility::ends_with( ' foo bar', ' bar' ) );
$this->assertFalse( StringUtility::ends_with( 'TEST', 'te', false ) );
$this->assertFalse( StringUtility::ends_with( ' FOO BAR', ' foo', false ) );
$this->assertTrue( StringUtility::ends_with( 'TEST', 'st', false ) );
$this->assertTrue( StringUtility::ends_with( ' FOO BAR', ' bar', false ) );
$this->assertFalse( StringUtility::ends_with( 'test', 'TE', false ) );
$this->assertFalse( StringUtility::ends_with( ' foo bar', ' FOO', false ) );
$this->assertTrue( StringUtility::ends_with( 'test', 'ST', false ) );
$this->assertTrue( StringUtility::ends_with( ' foo bar', ' BAR', false ) );
}
}