Skip to content

[DependencyInjection] [POC] allow ServiceSubscriberTrait to autowire properties #46617

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[DependencyInjection] [POC] allow ServiceSubscriberTrait to autowire properties #46617

wants to merge 1 commit into from

Conversation

kbond
Copy link
Member

@kbond kbond commented Jun 7, 2022 •
edited
Loading

Q A
Branch? 6.2
Bug fix? no
New feature? yes
Deprecations? no
Tickets n/a
License MIT
Doc PR todo

This comes from a slack discussion with @nicolas-grekas.

This PR allows ServiceSubscriberInterface services using the ServiceSubscriber trait to autowire properties marked with the SubscribedService attribute.

class MyService implements ServiceSubscriberInterface
{
    use ServiceSubscriberTrait;

    #[SubscribedService]
    private AnotherService $anotherService;

    public function someMethod()
    {
        $this->anotherService->something(); // lazily initializes the property the first time it's accessed
    }
}

TODO:

  • nullable properties?
  • more tests
  • changelog

@carsonbot carsonbot added this to the 6.2 milestone Jun 7, 2022
@carsonbot carsonbot changed the title [POC][DI] allow ServiceSubscriberTrait to autowire properties [DependencyInjection] [POC] allow ServiceSubscriberTrait to autowire properties Jun 7, 2022
@kbond kbond force-pushed the service-subscriber-properties branch 2 times, most recently from 187925e to deeb6fd Compare June 7, 2022 22:01
nicolas-grekas
nicolas-grekas reviewed Jul 28, 2022
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Many interesting challenges here, that's very much R&D :)

src/Symfony/Contracts/Service/ServiceSubscriberTrait.php Outdated
@@ -67,10 +99,28 @@ public function setContainer(ContainerInterface $container): ?ContainerInterface
{
$this->container = $container;

if ($container instanceof ServiceProviderInterface) {
// TODO: what if this isn't an instance of ServiceProviderInterface?
Copy link
Member

Choose a reason for hiding this comment

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

We should use reflection instead of relying on ServiceProviderInterface and unset only the properties that have the attribute

Copy link
Member Author

Choose a reason for hiding this comment

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

This would be at runtime, that ok?

src/Symfony/Contracts/Service/ServiceSubscriberTrait.php
{
// TODO: ensure cannot be called from outside of the scope of the object?
// TODO: what if class has a child/parent that allows this?
// TODO: call parent::__get()?
Copy link
Member

Choose a reason for hiding this comment

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

LazyGhostObjectTrait to the rescue somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not very familiar with this trait. How could it be used?

@kbond kbond force-pushed the service-subscriber-properties branch 3 times, most recently from 56b0944 to 573b694 Compare August 29, 2022 13:31
@kbond kbond force-pushed the service-subscriber-properties branch from 573b694 to e2fb640 Compare August 29, 2022 13:35
@nicolas-grekas nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 13, 2023 •
edited
Loading

I'm closing here because while this is an interesting topic, I feel like this would bring too much complexity in comparison to other approaches based on Autowire attributes, especially since #51392
Thanks for giving the idea a try!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

Assignees
No one assigned
Labels
DependencyInjection Feature Status: Needs Work
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
3 participants
@kbond @nicolas-grekas @carsonbot