feat(core): introduce tagged configurations

[innocenzi]

Tagged configurations

This pull request adds support for tagged configurations.

This is a new concept that attempts to solve the "multiple X" issue at the foundation level. This could be used, for instance, to support multiple databases by simply defining tagged configurations, and injecting a tagged instance of Database:

// database.backup.config.php
return new SQLiteConfig(
    path: __DIR__ . '/backup.sqlite',
    tag: 'backup',
);

// database.config.php
return new SQLiteConfig(
    path: __DIR__ . '/main.sqlite',
);

// some class
public function __construct(
    private readonly Database $mainDatabase,
    #[Tag('backup')]
    private readonly Database $backupDatabase,
) {}

 

Further considerations

Right now, this can only work because DynamicInitializers have been modified to accept a $tag. An initializer can determine if it will initialize a class as usual, but it can also resolve dependencies using the given tag, allowing to pass it down to the chain of dependencies.

This is quite verbose, since it would require an initializer for every class or interface for a feature that would implement this concept.

To avoid that, I added #[AllowDynamicTags] and #[ForwardTag]. The former prevents a class from not being resolved when it has a tag but no initializer and is not registered yet. The latter resolves the marked parameter using the same tag that was used for the parent class.

This is the least verbose pattern I could come up with.

I'm wondering though, if normal Initializer should also receive the tag. This would allow not using a DynamicInitializer for the Database, for instance. However, the API would become more confusing, with #[Singleton] accepting a tag.

 

Tags as enums

I have taken the opportunity to support enums in the #[Tag] attribute and all container methods accepting a $tag parameter, as this is usually better than specifying plain strings, which are prone to typos and not easily refactored.

$this->container->get(Service::class, tag: ExampleEnum::FOO);

 

Proof of concept

I included a proof of concept of the internal usage of tagged configurations by implementing it on Vite.

Currently, it's only possible to have one Vite configuration in a Tempest project—this means a you can't build multiple manifests or have multiple development servers while using the back-end integrations.

This is solved by specifying the tag property in ViteConfig, as well as replacing the default bridgeConfigFile, buildDirectory and manifest:

// vite.backoffice.config.php
return new ViteConfig(
    tag: 'backoffice',
    bridgeFileName: 'vite-tempest-backoffice',
    manifest: 'backoffice/manifest.json',
    buildDirectory: 'backoffice/build',
    entrypoints: [
        'src/BackOffice/main.ts',
    ],
);

// in a view
<x-vite-tags config="backoffice" />

Related to #339

[coveralls]

Pull Request Test Coverage Report for Build 14205916051

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 50 of 57 (87.72%) changed or added relevant lines in 12 files are covered.
• 3 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+0.09%) to 81.054%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/Tempest/Container/src/GenericContainer.php	31	33	93.94%
src/Tempest/Vite/src/ViteConfigCommand.php	0	5	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/Tempest/Database/src/Config/MysqlConfig.php	1	0.0%
src/Tempest/Database/src/Config/PostgresConfig.php	1	0.0%
src/Tempest/Vite/src/ViteConfigCommand.php	1	0.0%

Totals
Change from base Build 14184799467:	0.09%
Covered Lines:	11132
Relevant Lines:	13734

---

💛 - Coveralls

[brendt]

I really like the idea, but would like to brainstorm some more. Here are my thoughts:

1. Tagging a config file shouldn't be done with a magic variable on the config itself. What about some kind of helper function that wraps the config in a proxy?

// sqlite.config.php

return tag(new SqliteConfig(), 'sqlite');

Since config objects are registered in the container via the ->config method, we could easily check if we're dealing with eg. TaggedObject, and unwrap it there.

2. If we have this tag wrapper, I'm not sure if we'd still need the DynamicInitializer changes? Maybe I'm missing something
3. Tags as enums: makes sense 👍

[innocenzi]

I agree with the variable having to be specified in the config not being ideal. However, it's not that bad: for configurations like the database config, we could have a different name than tag to be more specific about what it is.

To be fair, I don't like the wrapper function. It's not convenient to type nor pretty:

return tag(new SQLiteConfig(
    path: __DIR__ . '/backup.sqlite',
), 'backup');

Also, no, this unfortunately not fix the DynamicInitializer changes. We definitely need the $tag to be accessible in initialize to instantiate the object's dependencies that rely on the tag:

final class DatabaseInitializer implements Initializer
{
    // ...

    #[Singleton]
    public function initialize(ClassReflector $class, Container $container, ?string $tag = null): Database
    {
        return new GenericDatabase(
            $container->get(Connection::class, $tag),
            $container->get(TransactionManager::class, $tag),
        );
    }
}

Even with #[AllowDynamicTags] and #[ForwardTag], there are cases like the above, where an initializer is needed and it needs to forward the tag it is being initialized with

[innocenzi]

@brendt as an alternative, I just imagined this syntax:

final class DatabaseInitializer implements Initializer
{
    #[CurrentTag]
    private ?string $tag = null;

    #[Singleton]
    public function initialize(Container $container): Database
    {
        return new GenericDatabase(
            $container->get(Connection::class, $this->tag),
            $container->get(TransactionManager::class, $this->tag),
        );
    }
}

This fixes the annoying signature changes, while still allowing to forward the tag the dependency is being initialized with. The dependency would need #[AllowDynamicTag]

[innocenzi]

@brendt the last commit is a proof of concept of the database working with multiple tagged configs. I feel like the API is still confusing but at least it's not verbose anymore because dynamic initializers are no longer needed: the current tag is injected into a property of the initializer, thanks to the #[CurrentTag] attribute. For this to work the initializer also has to have #[AllowDyanmicTags] though...

Overall I think the pattern (tagged configs and passthrough tags) is good but the API/code should be cleaned. Would love your help in this regard

[brendt]

Ah, we could add an interface? Taggable with a public string|array|UnitEnum $tag { get; } property? Then anything can be made taggable, but it's more explicit than relying on a random property with attribute?

I don't oppose the changes to DynamicInitializer, I just want to make sure I understand the need for them. That's what confuses me: I can't find the added $tag parameter being used in any of the dynamic initializers in your PR? You mention DatabaseInitializer, but that's not a dynamic initializer? Or is that just WIP?

[innocenzi]

Ah, we could add an interface?

Just to make sure, have you noticed the TaggedConfig interface?

I don't oppose the changes to DynamicInitializer

Here is what I did before the last commit: 2419252 (#1105)

Basically, the needs are as follow:

• Some interface or dependency allows "dynamic tags", so they can load associated "tagged configs" under the hood
• Users inject these dependencies and add a #[Tag] attribute with the same tag as the underlying tagged config
• Tempest now has to know what the tag is to inject the proper dynamic config
• This is done either through #[CurrentTag] and #[ForwardTag] or by specifying the tag as an argument to a DynamicInitializer so it can use it to resolve a config/dependency with the same tag

In the commit above, the user resolves Vite with any tag (eg. backoffice) and the initializer has to forward that tag to a config tagged backoffice as well, so the underlying code uses the right configuration

However, I personally am not a fan of having to use dynamic initializer just because we need a tag, which is why I added #[CurrentTag], and even better, #[ForwardTag] when we don't need an initializer at all

[innocenzi]

Hey @brendt, this is ready for review. If possible I'd like you to play with the feature, see how it stands conceptually, criticize the API—because this would be a fairly important part of future Tempest APIs.

The important parts:

• You can implement TaggedConfig on a configuration class, effectively allowing multiple instances of this configuration to coexist.
• You can add #[AllowDynamicTags] to any class and their initializer (if any). In this situation, a class can be resolved with any tag.
• You can add #[ForwardTag] to a constructor parameter to resolve this parameter using the same tag that was used to resolve the owner class.
• You can add #[CurrentTag] to a class property to inject it with the tag that was used to resolve the owner class. This is what allows us to keep using normal initializers to initialize dependencies with #[AllowDynamicTags].

[brendt]

Ok but I still don't understand this part:

You can add #[AllowDynamicTags] to any class and their initializer (if any). In this situation, a class can be resolved with any tag.
You can add #[ForwardTag] to a constructor parameter to resolve this parameter using the same tag that was used to resolve the owner class.
You can add #[CurrentTag] to a class property to inject it with the tag that was used to resolve the owner class. This is what allows us to keep using normal initializers to initialize dependencies with #[AllowDynamicTags].

Let me write up another PR to illustrate what I mean though, because I feel like we're going in circles.

[brendt]

Here's my PR: https://github.com/tempestphp/tempest-framework/pull/1120/files

What does your implementation do with the attributes that can't be solved with the interface?

[innocenzi]

What does your implementation do with the attributes that can't be solved with the interface?

It's completely unrelated 😅 Your HasTag interface is just a different abstraction to the TaggedConfig interface.

The rest of the implementation is less about tagged configurations and more about giving the framework the ability to initialize objects and their own dependencies with the same tag.

I recommend you try this PR out locally: create multiple database configs and inject them in a class with their own tag

[brendt]

Cross posting from Discord DMs

I gave it some more thought overnight. I've got two concerns:

• #[AllowDynamicTags] kind of needs to be used in tandem with #[CurrentTag], I don't like that two attributes depend on each other, that's super implicit. Yes, they can be used separately, but does that really make sense? In practice the two seem to be always combined? I think I would prefer an interface here. HasInjectableTag with a setTag method? Or, another approach would be to have an explicit Tag object that the container knows how to inject? So this:

#[AllowDynamicTags]
final class DatabaseInitializer implements Initializer
{
    #[CurrentTag]
    private ?string $tag; // @phpstan-ignore-line this is injected

Could be changed to this:

final class DatabaseInitializer implements Initializer
{
    private ?Tag $tag;

That feels a lot more in line to me with the standard way of how the container works.

We could also write it like this to be more explicit:

    #[Inject]
    private ?Tag $tag;

• My second issue is with #[ForwardTag], it is set on the "parent" dependency, which makes it unclear from the child's point of view that it's forwarded. So instead of this:

final class DependencyWithDynamicTag
{
    public ?Tag $tag;

   public function __construct(
        #[ForwardTag]
        public readonly SubdependencyWithDynamicTag $subependency,
    ) {}
}

final class SubdependencyWithDynamicTag
{
    public ?Tag $tag;
}

I'd rather write this:

final class DependencyWithDynamicTag
{
   public ?Tag $tag;

   public function __construct(
        public readonly SubdependencyWithDynamicTag $subependency,
    ) {}
}

final class SubdependencyWithDynamicTag
{
    #[InjectFromParent]
    public ?Tag $tag;
}

IDK, that feels more explicit to me, but maybe I'm overthinking it.

[innocenzi]

Closing this in favor of #1120