search

inpsyde / modularity

A PSR-11 implementation for WordPress Plugins, Themes or Libraries.
https://inpsyde.github.io/modularity/
GNU General Public License v2.0
46 stars 4 forks source link

Introduce ContainerCompiler #20

Closed gmazzap closed 1 year ago

gmazzap commented 2 years ago

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Feature.

What is the current behavior? (You can also link to an open issue here)

See details below.

What is the new behavior (if this is a feature change)?

See details below.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No.

The issue

Currently, Modularity uses an hardcoded depency to the ContainerConfigurator class to "collect" factories, services, extensions, and child containers, and then builds an instance of the ReadOnlyContainer PSR-11 implementation.

While this works most of the time, it make it impossible to use different PSR-11 implementation, for example to "decorate" the given container for debugging, logging, or similar things.

The proposed solution

The idea is to have a ContainerCompiler interface of which the current ContainerConfigurator would be one implementation (only implementation shipped with Modularity).

With an interface, consumers could provide their implementation and build a PSR-11 container as they like.

While the compiler enables very flexible operation, we believe most of the time the returned container should extend or decorate the provided ReadOnlyContainer to avoid breaking the package's "contract" with modules.

The implementation

To not break backward compatibility with the existing Package::new() method, we introduce a new Package::newWithCompiler() static constrcutor, that instead of accepting a number fo child container accepts instead a ready-made Compiler.

With such approach Package::new($properties, $container1, $container2) becomes equivalent to: Package::newWithCompiler( $properties, new ReadOnlyContainerCompiler($container1, $container2)), but consumers can use Package::newWithCompiler() to pass a different implementation of the new ContainerCompiler interface.

The default implemenation has been renamed from ContainerConfigurator to ReadOnlyContainerCompiler, but a class with the old name (extending the one with new name) is kept for backward compatibility. It is deprecated and will be removed in next major.

Use case examples

A filtered container

Do you like filters, don't you?

class FilteredContainer implements ContainerInterface
{
    public function __construct(private ContainerInterface $container)
    {
    }

    public function has(string $id): bool
    {
        return $this->container->has($id);
    }

    public function get(string $id): mixed
    {
        return apply_filters("my-container-service-{$id}", $this->container->get($id));
    }
}

class FilteredContainerCompiler extends ReadOnlyContainerCompiler
{
    public function compile(): ContainerInterface
    {
        return new FilteredContainer(parent::compile());
    }
}

Package::newWithCompiler($properties, new FilteredContainerCompiler());

Automatic LoggerAware resolution

As soon as a LoggerInterface object is available in the container, it injects it to every LoggerAware class that is resolved.

class LoggerAwareResolverContainer implements ContainerInterface
{
    public function __construct(private ContainerInterface $container)
    {
    }

    public function has(string $id): bool
    {
        return $this->container->has($id);
    }

    public function get(string $id): mixed
    {
        $object = $this->container->get($id);
        if ((!$object instanceof LoggerAwareInterface::class) || !$this->container->has(LoggerInterface::class)) {
            return $object;
        }
        $logger = $this->container->get(LoggerInterface::class);
        $object->setLogger($logger);

        return $object;
    }
}

class LoggerAwareResolverContainerCompiler extends ReadOnlyContainerCompiler
{
    public function compile(): ContainerInterface
    {
        return new LoggerAwareResolverContainer(parent::compile());
    }
}

Package::newWithCompiler($properties, new LoggerAwareResolverContainerCompiler());

Other notes

Currently, the ContainerConfigurator holds a container instance as property, and do not re-compile the container if it is already compiled. Now that we have an interface, we can't be sure the implementation will hold a property as well, and we don't want to risk having multiple container instances, or we defeat the benefits of having a container at all. So the property is moved to package.

The compiler is inject to the contructor and can't be changed later because it is needed right in the constructor, at least if we want to keep the Package::new() signature that accepts "child" containers. Otherwise we would not know where to store the child container nor the "properties" object. Even if we changed the Package::new() signature making an "impactful" breaking change, the compiler would still be needed before any addModule() call, hence the compiler "setter" would need to be called right after construction anyway. So we decided that it just make sense to accept it in a new static constructor an that's it.

Thanks to the fact we were wise enough to use a private constructor for the Package class, we can implement this change in a 100% backward compatible way.

Renaming ContainerConfigurator had been a breaking change. We can decide to have a new major and that's it, after all we don't expect ContainerConfigurator to be used in many places as independent object, or we can keep that class as deprecated and extend the new class. So far, this PR takes the latter approach, but that's not set in stone.

This PR includes unit tests and documentaion, which might be worth a read: https://github.com/inpsyde/modularity/blob/feature/container-compiler-interface/docs/PSR-11-Container.md

Alternatives

Like the examples above shows, most of the usage of the compiler we can think of is about decorating the default ReadOnlyContainer. An alternative we considered si to have a ContainerDecorator object, configurable to package via a setter, which if provided could be used to decorate the default container, having a single method with signature ContainerDecorator::decorate(ContainerInterface $c): ContainerInterface.

That would be less impactful because we don't need a new constructor nor any deprecation and renaming of ContainerConfigurator which would stay just as is.

On the other had, it would be less flexible, and won't solve the hardcoded ContainerConfigurator dependency which isn't exaclty an example of good software design.

Feedback wanted

Of course, like any other PR, this is open to any kind of feedback.

However, here's below a few questions that could help the conversation:

Chrico commented 1 year ago

As discussed, for me this apporach is totally fine. 👍🏻

Naming: are ContainerCompiler for the interface and ReadOnlyContainerCompiler for the implementation good names? Better alternatives?

For me Compiler, Builder and Configurator is right here. We choosed when creating this library "Configurator", eventho we internally always said "Compile the PSR Container".

Do you agree in renaning current ContainerConfigurator?

I would rename it + move it out of the Container-Domain into an own one ( Compiler ?). The Container\ContainerConfigurator is only used internally and never accessed externally. Eventho renaming/moving is a breaking change...no one accesses that internal code.

What do you think of the "alternative" implementation (using ContainerDecorator)? + In a scale of 0 (not helpful at all) to 5 (fundamental) how much think this PR would be helpful in projects you're working on?

Could be also a feasible solution. TBH I've no real world use case right now where I would need that kind of "Decorator" or "custom Compiler". Everything works fine for me and the Package itself provides enough information via Package::moduleStatus(). Everything else like tracing, logging or debugging shouldn't be really done inside of the Container when resolving/registering Services. This could have an even bigger performance impact.

codecov[bot] commented 1 year ago

Codecov Report

Base: 96.24% // Head: 95.37% // Decreases project coverage by -0.87% :warning:

Coverage data is based on head (09d19e0) compared to base (65bc3f8). Patch coverage: 95.91% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #20 +/- ## ============================================ - Coverage 96.24% 95.37% -0.88% - Complexity 177 180 +3 ============================================ Files 9 10 +1 Lines 426 432 +6 ============================================ + Hits 410 412 +2 - Misses 16 20 +4 ``` | Flag | Coverage Δ | | |---|---|---| | unittests | `95.37% <95.91%> (-0.88%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=inpsyde#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/inpsyde/modularity/pull/20?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=inpsyde) | Coverage Δ | | |---|---|---| | [src/Container/ContainerConfigurator.php](https://codecov.io/gh/inpsyde/modularity/pull/20/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=inpsyde#diff-c3JjL0NvbnRhaW5lci9Db250YWluZXJDb25maWd1cmF0b3IucGhw) | `0.00% <0.00%> (-100.00%)` | :arrow_down: | | [src/Container/ReadOnlyContainerCompiler.php](https://codecov.io/gh/inpsyde/modularity/pull/20/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=inpsyde#diff-c3JjL0NvbnRhaW5lci9SZWFkT25seUNvbnRhaW5lckNvbXBpbGVyLnBocA==) | `100.00% <100.00%> (ø)` | | | [src/Package.php](https://codecov.io/gh/inpsyde/modularity/pull/20/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=inpsyde#diff-c3JjL1BhY2thZ2UucGhw) | `97.85% <100.00%> (+0.06%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=inpsyde). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=inpsyde)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

gmazzap commented 1 year ago

@Chrico

I would rename it + move it out of the Container-Domain into an own one ( Compiler ?).

Why that? The "compiler" is not a "generic" compiler. It is a container compiler. So to me, it belongs semantically to the container.

It is only used internally and never accessed externally. Eventho renaming/moving is a breaking change...no one accesses that internal code.

Yes, but you don't know who outside of the library is using it and how. If you rename a class, a new major is required. So IMO, we should decide if to keep the current ContainerConfigurator so we can avoid having a major release or go with a new major by deleting ContainerConfigurator. In that case, the problem is that we probably want to maintain only the latest major, so many packages using Modularity will be using an unmaintained version.

Could be also a feasible solution. TBH I've no real world use case right now where I would need that kind of "Decorator" or "custom Compiler".

Me neither, but after I've played a bit with possible usages of this PR, I think the compiler provides better flexibility. So if we decide to merge this, I think we should use the "compiler" approach and not the "decorator" approach.

gmazzap commented 1 year ago

After some more internal discussion, we decided to close this.