Skip to content

Commit

Permalink
Settled on an injection method of init
Browse files Browse the repository at this point in the history
1. Since our code style dictates that these be `final` methods, we shouldn't be concerned about overlap.
2. There is precedent for `init` methods as requirements before using class instances.
  • Loading branch information
ObliviousHarmony committed Aug 7, 2020
1 parent 154c812 commit 081f0d9
Show file tree
Hide file tree
Showing 11 changed files with 45 additions and 27 deletions.
4 changes: 2 additions & 2 deletions phpcs.xml
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
<include-pattern>src/</include-pattern>
<include-pattern>tests/php/src/</include-pattern>
<properties>
<property name="injectionMethod" value="container_init"/>
<property name="injectionMethod" value="init"/>
</properties>
</rule>

Expand Down Expand Up @@ -88,7 +88,7 @@
<rule ref="Squiz.Commenting.FileComment.Missing">
<exclude-pattern>tests/php/</exclude-pattern>
</rule>

<rule ref="WordPress.NamingConventions.ValidFunctionName.MethodNameInvalid">
<exclude-pattern>src/</exclude-pattern>
</rule>
Expand Down
2 changes: 1 addition & 1 deletion src/Internal/DependencyManagement/Definition.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class Definition extends BaseDefinition {
/**
* The standard method that we use for dependency injection.
*/
const INJECTION_METHOD = 'container_init';
const INJECTION_METHOD = 'init';

/**
* Resolve a class using method injection instead of constructor injection.
Expand Down
3 changes: 1 addition & 2 deletions src/Proxies/LegacyProxy.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@
namespace Automattic\WooCommerce\Proxies;

use Automattic\WooCommerce\Internal\DependencyManagement\Definition;
use \Psr\Container\ContainerInterface as Container;
use Symfony\Component\DependencyInjection\ContainerInterface;
use \Psr\Container\ContainerInterface;

/**
* Proxy class to access legacy WooCommerce functionality.
Expand Down
18 changes: 9 additions & 9 deletions src/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 `container_init` arguments, which are automatically supplied by the container when the class is resolved (this is called _dependency injection_).
* Classes in the `src` directory specify their dependencies as `init` 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
Expand All @@ -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 method injection. This means specifying these dependencies as arguments in a `container_init` method 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 `init` method with appropriate type hints, and storing these in private variables, ready to be used when needed:

```php
use TheService1Namespace\Service1;
Expand All @@ -89,7 +89,7 @@ class TheClassWithDependencies {

private $service2;

public function container_init( Service1Class $service1, Service2Class $service2 ) {
public function init( Service1Class $service1, Service2Class $service2 ) {
$this->$service1 = $service1;
$this->$service2 = $service2;
}
Expand All @@ -110,7 +110,7 @@ use TheService1Namespace\Service1;
class TheClassWithDependencies {
private $container;

public function container_init( \Psr\Container\ContainerInterface $container ) {
public function init( \Psr\Container\ContainerInterface $container ) {
$this->$container = $container;
}

Expand Down Expand Up @@ -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 `container_init` 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 `init` arguments in detailed above.

Example of such a class:

Expand All @@ -158,7 +158,7 @@ use Automattic\WooCommerce\TheDependencyNamespace\TheDependencyClass;
class TheClass {
private $the_dependency;

public function container_init( TheDependencyClass $dependency ) {
public function init( TheDependencyClass $dependency ) {
$this->the_dependency = $dependency;
}

Expand Down Expand Up @@ -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 `container_init` 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 `container_init` 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 `init` 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 `init` 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:

Expand Down Expand Up @@ -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 dependency-injected, and thus must not be used as type hints in `container_init` 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 `init` 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
Expand Down Expand Up @@ -306,7 +306,7 @@ use Automattic\WooCommerce\Proxies\LegacyProxy;
class TheClass {
private $legacy_proxy;

public function container_init( LegacyProxy $legacy_proxy ) {
public function init( LegacyProxy $legacy_proxy ) {
$this->legacy_proxy = $legacy_proxy;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,14 @@ class ClassWithDependencies {
public $dependency_class = null;

/**
* Sets the dependencies for the class.
* Initialize the class instance.
*
* @internal
*
* @param DependencyClass $dependency_class A class we depend on.
* @param int $some_number Some number we need for some reason.
*/
final public function container_init( DependencyClass $dependency_class, int $some_number = self::SOME_NUMBER ) {
final public function init( DependencyClass $dependency_class, int $some_number = self::SOME_NUMBER ) {
self::$instances_count++;
$this->dependency_class = $dependency_class;
$this->some_number = self::SOME_NUMBER;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@
class ClassWithInjectionMethodArgumentWithoutTypeHint {

/**
* Sets class dependencies.
* Initialize the class instance.
*
* @internal
*
* @param mixed $argument_without_type_hint Anything, really.
*/
final public function container_init( $argument_without_type_hint ) {
final public function init( $argument_without_type_hint ) {
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
<?php
/**
* ClassWithNonFinalInjectionMethod class file.
*
* @package Automattic\WooCommerce\Tests\Internal\DependencyManagement\ExampleClasses
*/

namespace Automattic\WooCommerce\Tests\Internal\DependencyManagement\ExampleClasses;
Expand All @@ -10,9 +12,13 @@
*/
class ClassWithNonFinalInjectionMethod {

// phpcs:disable WooCommerce.Functions.InternalInjectionMethod.MissingFinal

/**
* Sets class dependencies.
* Initialize the class instance.
*
* @internal
*/
public function container_init() {
public function init() {
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,13 @@
*/
class ClassWithPrivateInjectionMethod {

// phpcs:disable WooCommerce.Functions.InternalInjectionMethod.MissingPublic

/**
* Sets class dependencies.
* Initialize the class instance.
*
* @internal
*/
final private function container_init() {
final private function init() {
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@ class ClassWithScalarInjectionMethodArgument {
// phpcs:disable Squiz.Commenting.FunctionComment.InvalidTypeHint

/**
* Sets class dependencies.
* Initialize the class instance.
*
* @internal
*
* @param mixed $scalar_argument_without_default_value Anything, really.
*/
final public function container_init( int $scalar_argument_without_default_value ) {
final public function init( int $scalar_argument_without_default_value ) {
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
* 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 {

/**
* The injected LegacyProxy.
*
Expand All @@ -21,11 +22,13 @@ class ClassThatDependsOnLegacyCode {
private $legacy_proxy;

/**
* Sets class dependencies.
* Initialize the class instance.
*
* @internal
*
* @param LegacyProxy $legacy_proxy The instance of LegacyProxy to use.
*/
public function container_init( LegacyProxy $legacy_proxy ) {
final public function init( LegacyProxy $legacy_proxy ) {
$this->legacy_proxy = $legacy_proxy;
}

Expand Down
2 changes: 1 addition & 1 deletion tests/php/src/Proxies/LegacyProxyTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public function setUp() {
*/
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 ' . Definition::INJECTION_METHOD . ' method 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 ' . Definition::INJECTION_METHOD . ' method injection or the instance of Psr\Container\ContainerInterface for that.' );

$this->sut->get_instance_of( DependencyClass::class );
}
Expand Down

0 comments on commit 081f0d9

Please sign in to comment.