Skip to content

[DependencyInjection] Add support for casting callables into single-method interfaces #49632

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
Merged
merged 1 commit into from
Mar 28, 2023
Merged

[DependencyInjection] Add support for casting callables into single-method interfaces #49632

merged 1 commit into from
Mar 28, 2023

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Mar 7, 2023 •
edited
Loading

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

This PR makes it possible to cast closures to single-method interfaces.

It is best reviewed ignoring whitespaces.

This works at the service definition level. Here is an example that creates heterogeneous URI-template expanders and exposes them under a common interface (using real classes inspired from #49302):

interface UriExpanderInterface
{
    public function expand(string $url, array $vars): string;
}

And then, in some service configuration using the PHP-DSL:

    ->set('uri_expander.guzzle', UriExpanderInterface::class)
        ->fromCallable([\GuzzleHttp\UriTemplate\UriTemplate::class, 'expand'])

    ->set('uri_template_expander.rize', UriExpanderInterface::class)
        ->fromCallable([inline_service(\Rize\UriTemplate::class), 'expand'])

    ->alias(UriExpanderInterface::class, 'uri_template_expander.rize' or 'uri_template_expander.guzzle')

Internally, this creates a proxy class that will behave like this one (which could be used in a test case):

$expander = new class ($closure) implements UriExpanderInterface {
    public function __construct(private \Closure $closure)
    {
    }

    public function expand(string $url, array $vars): string
    {
        return $this->closure->__invoke($url, $vars);
    }
};

This also adds support for YAML and XML of course:

services:
    uri_template_expander.rize:
        class: UriExpanderInterface
        from_callable: [ !service {class: Rize\UriTemplate}, expand ]
<services>
  <service id="uri_template_expander.rize" class="UriExpanderInterface">
    <from-callable method="expand">
      <service class="Rize\UriTemplate"/>
    </from-callable>
  </service>
</services>

This also works using attributes:

class MyClass
{
    public function __construct(
        #[AutowireCallable([\GuzzleHttp\UriTemplate\UriTemplate::class, 'expand'])]
        UriExpanderInterface $expander,
    )
}

@carsonbot carsonbot added this to the 6.3 milestone Mar 7, 2023
@Neirda24
Copy link
Contributor

Neirda24 commented Mar 7, 2023

Doesn't this reduce the evolutivity of the interface? If I understand this correctly then adding a second method to the interface would break your wrapping. I don't see the real benefits at the moment. Could you share real world usage cases ?

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Mar 8, 2023 •
edited
Loading

@Neirda24 adding a method to an interface is a BC break so this shouldn't come unplanned. Also the segregation principle tells us that interfaces should have less methods, not more, so that the natural path for them is to shrink, not grow. Of course nothing enforces this, but this still helps answer you argument. I don't have a better real world usage than the one I posted in the description.

@Neirda24
Copy link
Contributor

Neirda24 commented Mar 8, 2023

@nicolas-grekas : agreed on the principles. I simply fear that it adds a lot of complexity both to maintain in symfony and to read in real world apps without gaining much from it. Maybe there is something I am not seeing ATM. Using the factory we usually expect that the class or type is the one from the method itself, here it is automatically cast as what Interface was provided on the service definition. AFAIK only this factory would do this. Am I wrong ?

@nicolas-grekas nicolas-grekas force-pushed the di-fn2if branch 2 times, most recently from ac79179 to 1e609b0 Compare March 8, 2023 11:13
@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Mar 8, 2023 •
edited
Loading

PR updated to make it easy to configure. This should alleviate your concerns @Neirda24. About the complexity for maintainers, this was pretty straightforward to implement actually so it's not an issue to me.

@Neirda24
Copy link
Contributor

Neirda24 commented Mar 8, 2023

Indeed easier to read. And less confusing for new developers. Gotta make sure the "cast" keyword is kept out of the documentation because even though it could be affiliated to a cast it is more a decorator to me. Thanks @nicolas-grekas !

@nicolas-grekas nicolas-grekas force-pushed the di-fn2if branch 2 times, most recently from 981b377 to 42618d2 Compare March 9, 2023 16:12
@chalasr
Copy link
Member

chalasr commented Mar 13, 2023

rebase needed

@nicolas-grekas
Copy link
Member Author

done

chalasr
chalasr approved these changes Mar 13, 2023
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Cool

@nicolas-grekas nicolas-grekas force-pushed the di-fn2if branch 3 times, most recently from f8ff913 to a20f329 Compare March 20, 2023 15:58
@nicolas-grekas
Copy link
Member Author

(rebased)

@nicolas-grekas nicolas-grekas changed the title [DependencyInjection] Make it possible to cast closures to single-method interfaces [DependencyInjection] Make it possible to cast callable to single-method interfaces Mar 24, 2023
@nicolas-grekas nicolas-grekas changed the title [DependencyInjection] Make it possible to cast callable to single-method interfaces [DependencyInjection] Add support for casting callable to single-method interfaces Mar 24, 2023
@nicolas-grekas nicolas-grekas requested a review from dunglas as a code owner March 24, 2023 01:06
@nicolas-grekas nicolas-grekas changed the title [DependencyInjection] Add support for casting callable to single-method interfaces [DependencyInjection] Add support for casting callables into single-method interfaces Mar 27, 2023
@nicolas-grekas nicolas-grekas force-pushed the di-fn2if branch 3 times, most recently from 8a74fd9 to be26be8 Compare March 27, 2023 14:53
@nicolas-grekas nicolas-grekas force-pushed the di-fn2if branch 2 times, most recently from f54e352 to ad0c643 Compare March 27, 2023 21:01
@nicolas-grekas nicolas-grekas merged commit b6e9b28 into symfony:6.3 Mar 28, 2023
@nicolas-grekas nicolas-grekas deleted the di-fn2if branch March 28, 2023 09:55
@nicolas-grekas nicolas-grekas mentioned this pull request Mar 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chalasr chalasr chalasr approved these changes

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

Assignees
No one assigned
Labels
DependencyInjection Feature Status: Needs Review Status: Reviewed
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.
4 participants
@nicolas-grekas @Neirda24 @chalasr @carsonbot