Introduce service extensions by type

[gmazzap]

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

• The commit message follows our guidelines
• Tests for the changes have been added (for bug fixes/features)
• Docs have been added/updated (for bug fixes/features)

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]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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

@@             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	Coverage Δ
unittests	98.97% <100.00%> (+0.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tfrommen]

So, I've come back to this several times, thinking about the use cases, and gotchas...
Code-wise, I think this is looking good.
I can see two different use cases, in general, but I'm not sure which others would be possible or sensible.

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

I added a few comments to the readme.

The updated/added tests, however, are more on the integration level, to me. Before this PR, the service extensions were stored in an assotciated array, so they were an actual part of the container. But now, with ServiceExtensions as a dedicated class, I am wondering if the unit tests should use a simple array-like mock instead, and if there should be dedicated unit tests for the new class. (It seems that you still treat the extensions "collection" as a part of the container.)

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? 😉

[gmazzap]

@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]

@tfrommen introduceced BC compat in a37ec01

[Chrico]

Went through the changes and just added 2 comments. Nothing really blocking, but we might should quickly talk about it. :)