Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add autowire/autodiscovery/auto-initialization of interfaces to classes #501

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

Jeroen-G
Copy link
Contributor

@Jeroen-G Jeroen-G commented Sep 28, 2024

Rationale

First of all, it is exciting to play with this new framework! I like a lot of it already, but I hit one snag when building some services. I usually have an interface for them (for example to make mocking easier). But with Tempest it looks like this would require a initializer class. And I am too lazy to make those for all my services 😜
A while ago I made an Autowire implementation for Laravel (link) so I thought I would start porting that a little bit to Tempest. So this first PR adds the Autowire attribute which allows autodiscovery of classes (or maybe auto initialization is a name better fitting to Tempest).

Let me know what you think!

How to use it

GreetingController:

final readonly class GreetingController
{
    public function __construct(public GreetingInterface $service)
    {}

    #[Get('/greetings')]
    public function index(): View
    {
        return view('greeting.view.php')->data(greeting: $this->service->greeting());
    }
}

GreetingInterface:

interface GreetingInterface
{
    public function greeting(): string;
}

GreetingService:

use Tempest\Container\Autowire;

#[Autowire]
final class GreetingService implements GreetingInterface
{
    public function greeting(): string
    {
        return 'hello world';
    }
}

@Jeroen-G Jeroen-G changed the title feat: add autowire/autodiscovery of interfaces to classes feat: add autowire/autodiscovery/auto-initialization of interfaces to classes Sep 28, 2024
@brendt
Copy link
Member

brendt commented Sep 29, 2024

Oooh, I like this! I'll probably review the PR tomorrow, but I'm pretty confident I'll merge it :)

@coveralls
Copy link

coveralls commented Sep 29, 2024

Pull Request Test Coverage Report for Build 11112271316

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.

Details

  • 15 of 26 (57.69%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.09%) to 81.385%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Tempest/Container/src/AutowireDiscovery.php 10 21 47.62%
Totals Coverage Status
Change from base Build 11062628894: -0.09%
Covered Lines: 5946
Relevant Lines: 7306

💛 - Coveralls

@yassiNebeL
Copy link
Contributor

yassiNebeL commented Sep 29, 2024

Oooh, I like this! I'll probably review the PR tomorrow, but I'm pretty confident I'll merge it :)

I appreciate your contribution @Jeroen-G! However, Tempest already has a Singleton attribute that handles service injection directly into the container. Could you clarify how this autowiring feature complements or improves upon that, and whether it addresses scenarios where the existing attribute falls short?

@Jeroen-G
Copy link
Contributor Author

Oooh, I like this! I'll probably review the PR tomorrow, but I'm pretty confident I'll merge it :)

I appreciate your contribution @Jeroen-G! However, Tempest already has a Singleton attribute that handles service injection directly into the container. Could you clarify how this autowiring feature complements or improves upon that, and whether it addresses scenarios where the existing attribute falls short?

Did you try out the example in the PR description? That is something the Singleton attribute cannot do. Automatically wiring interfaces to implementation is different from registering a singleton implementation. So that is where this feature complements upon :)

@Jeroen-G
Copy link
Contributor Author

Could you give a code example?

@shaffe-fr
Copy link
Contributor

This PR allows to tell the container: when I want a GreetingInterface give me this implementation.

Copy link
Member

@brendt brendt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

I've got one idea though I wanted to pitch though, but I'm not sure I like it myself.

I don't really like too many attributes on a class. I was wondering if it would make sense if we combined the autowiring attribute and singleton attribute into one, something like:

#[Singleton(for: Interface::class)]

I don't think it's better than this, but I wanted to pitch the idea just in case.

src/Tempest/Container/src/AutoDiscovery.php Outdated Show resolved Hide resolved
src/Tempest/Container/src/AutoDiscovery.php Outdated Show resolved Hide resolved
@Jeroen-G
Copy link
Contributor Author

This is great!

I've got one idea though I wanted to pitch though, but I'm not sure I like it myself.

I don't really like too many attributes on a class. I was wondering if it would make sense if we combined the autowiring attribute and singleton attribute into one, something like:

#[Singleton(for: Interface::class)]

I don't think it's better than this, but I wanted to pitch the idea just in case.

Pragmatically I could go with this. Conceptually however a singleton is meant to restrict instantiation of a class to one instance, which I think is not necessarily the case here. If you would like to merge them, I would propose:

#[Autowire(singleton: true)]

Semi-related, if this gets merged I would also look into opening a new PR for a Configure attribute.

@brendt
Copy link
Member

brendt commented Oct 1, 2024

I actually think just making the combination of both attributes #[Autowire] and #[Singleton] will be the best approach, as it causes the least confusion, and prevents one thing from being done in two ways.

Not sure yet how I feel about #[Configure], but let's open a new discussion for it :)

Btw: are you on Discord?

@brendt
Copy link
Member

brendt commented Oct 1, 2024

I think we're ready to merge? Can you rebase the branch on main?

@brendt brendt merged commit 1572122 into tempestphp:main Oct 1, 2024
53 checks passed
@brendt
Copy link
Member

brendt commented Oct 1, 2024

Super nice! I'll write some docs for this as well :)

@Jeroen-G
Copy link
Contributor Author

Jeroen-G commented Oct 1, 2024

Cool! Thanks, and I see you already found my discord handle :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants