inpsyde / modularity

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

Introduce service extensions by type #44

Closed gmazzap closed 4 months ago

gmazzap commented 4 months ago

This PR introduces the possibility for ExtendingModules to declare extensions not only by specific ID but also by type.

Services' extension handling has been moved to a separate class (Container\ServiceExtensions) to facilitate and decouple the handling of the new, more advanced behavior.

An example taken from the updated docs:

class LoggerAwareExtensionModule implements ExtendingModule
{
    use ModuleClassNameIdTrait;

    public function extensions() : array 
    {
        return [
            '@instanceof<' . LoggerAwareInterface::class . '>' => static function(
                LoggerAwareInterface $service,
                ContainerInterface $c
            ): ExtendedService {

                if ($c->has(LoggerInterface::class)) {
                    $service->setLogger($c->get(LoggerInterface::class));
                }

                return $service;
            }
        ];
    }
}

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)

Extensions need to target a specific service ID.

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

Extensions can also target a service type.

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

No.

Other information:

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.97%. Comparing base (7538809) to head (75aa539).

:exclamation: Current head 75aa539 differs from pull request most recent head ca70c43. Consider uploading reports for the commit ca70c43 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #44 +/- ## ============================================ + Coverage 98.87% 98.97% +0.10% - Complexity 192 217 +25 ============================================ Files 9 10 +1 Lines 531 588 +57 ============================================ + Hits 525 582 +57 Misses 6 6 ``` | [Flag](https://app.codecov.io/gh/inpsyde/modularity/pull/44/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=inpsyde) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/inpsyde/modularity/pull/44/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=inpsyde) | `98.97% <100.00%> (+0.10%)` | :arrow_up: | 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.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

gmazzap commented 4 months ago

@tfrommen Thank you.

Did you introduce this for decorating instances of specific "types"? Using setters, or with-methods (or builder methods), or constructor based decoration...?

I think the best real-world scenario is the PSR-3 LoggerAwareInterface. It defines a withLogger() method. The PSR-3 repo already provides a trait so you can do class X implements LoggerAwareInterface { use LoggerAwareTrait; } and you're done from a code perspective, still you are waiting for "something" to push the logger.

Often the logger is defined at website/application level, while "logger aware" objects are defined in plugins/themes/whatever. So you might have a plugin with 20/30 or more services all declared like:

Something::class => function (Container $c): Something {
   $instance = new Something();
   $instance->setLogger($c->get(Loggerinterface::class));
   return $instance;
},

It's 2 lines of code, but repeated dozen of times. With this change you don't need to do that.

At website/application level, you define both the logger and the by-type extension that inject the logger to anything that is LoggerAwareInterface. Then, at plugin/theme/library level, you only need to declare the object as LoggerAwareInterface and the logger will be injected.

More generally speaking, working with a two-layered applications, where some services are defined at website level, and some others at plugin/theme/library level, and the latter consumes the previous, having a method like that might be useful in similar situations, especially with PSRs that often rely on "withers" to configure objects.

I don't know, we could use this to add a header to any PSR client request sent to a specific domain. There are many possible use cases (besides the logger use case that was an immediate need).


the updated/added tests, however, are more on the integration level, to me

I guess how we define a "unit". We are not integrating anything that is outside this package, so not sure these are "integration" tests. We could maybe call it "functional" as we are testing a "feature" more than a single thing, but again, I'm not a fan of definining "unit" tests only if it tests a single class. A "unit" might be larger if you defien it so, imo. That said, having tests only for the specific class is definetively something that can be added.


This PR includes a breaking change to the constructor of ReadOnlyContainer, so this requires a major version bump. Is there anything else (related) that we want to "break" now?

True. It seems I pushed to quickly something that was on my HDD for long time and missed to see this. I guess it is better to work in a backward compatible way :) It will be a bit more code, but better than forcing a major.

gmazzap commented 4 months ago

@tfrommen introduceced BC compat in a37ec01