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

[DI] Add support for "wither" methods - for greater immutable services #30212

Merged
merged 1 commit into from Apr 3, 2019

Conversation

@nicolas-grekas
Copy link
Member

commented Feb 12, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR symfony/symfony-docs#10991

Let's say we want to define an immutable service while still using traits for composing its optional features. A nice way to do so without hitting the downsides of setters is to use withers. Here would be an example:

 class MyService
{
    use LoggerAwareTrait;
}

trait LoggerAwareTrait
{
    private $logger;

    /**
     * @required
     * @return static
     */
    public function withLogger(LoggerInterface $logger)
    {
        $new = clone $this;
        $new->logger = $logger;

        return $new;
    }
}

$service = new MyService();
$service = $service->withLogger($logger);

As you can see, this nicely solves the setter issues.

BUT how do you make the service container create such a service? Right now, you need to resort to complex gymnastic using the "factory" setting - manageable for only one wither, but definitely not when more are involved and not compatible with autowiring.

So here we are: this PR allows configuring such services seamlessly.
Using explicit configuration, it adds a 3rd parameter to method calls configuration: after the method name and its parameters, you can pass true and done, you just declared a wither:

services:
    MyService:
        calls:
            - [withLogger, ['@logger'], true]

In XML, you could use the new returns-clone attribute on the <call> tag.

And when using autowiring, the code looks for the @return static annotation and turns the flag on if found.

There is only one limitation: unlike services with regular setters, services with withers cannot be part of circular loops that involve calls to wither methods (unless they're declared lazy of course).

@nicolas-grekas nicolas-grekas added this to the next milestone Feb 12, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-withers branch from 33bd906 to 0f5a1c8 Feb 12, 2019

@rvanlaak

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

The third parameter does not feel as explicit as it should.

On the other hand is the following format a bit overkill:

services:
    MyService:
        with:
            - [withLogger, '@logger']

Did you think about some other formats as well?

@tsantos84

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

Its ok to access private members as public ones? Sounds weird to me. Its ok this approach in this context?

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-withers branch from 0f5a1c8 to 82a2d2f Feb 12, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

Did you think about some other formats as well?

None that clicked - that's the most simple I could think of - like for you, "with" doesn't click. I'm good with the 3rd arg - withers already start with "with" and that's enough maybe.

Its ok to access private members as public ones? Sounds weird to me. Its ok this approach in this context?

of course it is, that's pretty common for this and similar cases.

@Tobion

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

services:
    MyService:
        calls:
            - [withLogger, '@logger', true]

It does not look like this works. The second array element needs to be an array of arguments (['@logger']). Ref. #25663
And the third element was not implemented in YamlFileLoader as far as I see (which is good IMO).

So it needs to be written in the following way which is also alot more readable:

services:
    MyService:
        calls:
            - 
                method: withLogger
                arguments: ['@logger']
                use_result: true

I would go so far that we should think about deprecating the yaml syntax with array list in favour of named properties (#13892)

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-withers branch 2 times, most recently from 4ce5f60 to 1e41685 Feb 13, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

Thanks @Tobion and everyone, comments addressed, PR ready.

I would go so far that we should think about deprecating the yaml syntax with array list in favour of named properties

that's too far for me :) but that can be discussed in a RFC if you want to.

@fancyweb

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

Shouldn't there be a check that the "wither" method actually returns an instance of the same class ? What prevents people from returning something else ?

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

That's not the job of a DI container, eg we don't check you won't inject a logger where an event dispatcher is expected. That's a job for the language runtime, or a separate compiler pass / inspection process.

@beoboo

beoboo approved these changes Feb 20, 2019

Copy link

left a comment

I tried this locally and works very well.

@stof

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

On the other hand is the following format a bit overkill:

services:
    MyService:
        with:
            - [withLogger, '@logger']

Did you think about some other formats as well?

@rvanlaak Registering withers separately from other calls would have a big drawback: if you have both with and calls, what is their order ? Do we call methods on the initial object or on the final one ?
Marking calls themselves as wither allows to support any order (even having withers in the middle of non-wither calls) as the order is explicit.

@@ -136,15 +136,27 @@ protected function processValue($value, $isRoot = false)
}
$this->lazy = false;
// Any calls before a "wither" are part of the constructor-instantiation graph

This comment has been minimized.

Copy link
@stof

stof Feb 20, 2019

Member

Aren't any call to a non-wither performed before a wither call also part of it ?

This comment has been minimized.

Copy link
@stof

stof Feb 20, 2019

Member

Please add some tests covering mixing wither calls with non-wither calls (in different orders) to make sure this works right.

This comment has been minimized.

Copy link
@zanbaldwin

zanbaldwin Feb 20, 2019

Contributor

I would argue that non-wither calls should happen after wither calls, on the off chance that some calls register themselves with other services and the object hashes differ and causes unintended problems.

This comment has been minimized.

Copy link
@stof

stof Feb 20, 2019

Member

Well, I would say that mixing things are an edge case indeed. But the current configuration syntax supports it, and so we should deal with it properly.

The code currently already respects the order of method calls, be them withers or no, when instantiating the service. Only this place is missing that.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 20, 2019

Author Member

Aren't any call to a non-wither performed before a wither call also part of it ?

sure, that'swhat I mean by "any calls" - regular setters or withers

non-wither calls should happen after wither calls

why not, but not something I would make a "must"

Show resolved Hide resolved src/Symfony/Component/DependencyInjection/Definition.php
Show resolved Hide resolved ...ony/Component/DependencyInjection/Tests/Fixtures/php/services_wither.php Outdated

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-withers branch 2 times, most recently from bb5c76d to 4c6ad1e Mar 12, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

@stof thanks for the review, issue addressed.

Statuts: needs review

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-withers branch 2 times, most recently from eeb92bf to 12787bf Mar 12, 2019

@nicolas-grekas nicolas-grekas requested a review from stof Mar 12, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

friendly ping @symfony/deciders

@fabpot
Copy link
Member

left a comment

Looks good to me. Not a big fan of use-result though. use-returned-value would be more explicit IMHO, but perhaps a bit verbose. What about use-returned? Or anyone with a better idea?

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

use-result? use-returned-value? use-returned?
is-wither-factory? is-wither?
as-factory? is-factory?
is-wither-factory?

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-withers branch from 12787bf to f455d1b Apr 3, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

PR and description updated to use returns-clone, as discussed on Slack.

@fabpot

fabpot approved these changes Apr 3, 2019

@chalasr

chalasr approved these changes Apr 3, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

Thank you @nicolas-grekas.

@fabpot fabpot merged commit f455d1b into symfony:master Apr 3, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 3, 2019

feature #30212 [DI] Add support for "wither" methods - for greater im…
…mutable services (nicolas-grekas)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[DI] Add support for "wither" methods - for greater immutable services

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | symfony/symfony-docs#10991

Let's say we want to define an immutable service while still using traits for composing its optional features. A nice way to do so without hitting [the downsides of setters](https://symfony.com/doc/current/service_container/injection_types.html#setter-injection) is to use withers. Here would be an example:

```php
 class MyService
{
    use LoggerAwareTrait;
}

trait LoggerAwareTrait
{
    private $logger;

    /**
     * @required
     * @return static
     */
    public function withLogger(LoggerInterface $logger)
    {
        $new = clone $this;
        $new->logger = $logger;

        return $new;
    }
}

$service = new MyService();
$service = $service->withLogger($logger);
```

As you can see, this nicely solves the setter issues.

BUT how do you make the service container create such a service? Right now, you need to resort to complex gymnastic using the "factory" setting - manageable for only one wither, but definitely not when more are involved and not compatible with autowiring.

So here we are: this PR allows configuring such services seamlessly.
Using explicit configuration, it adds a 3rd parameter to method calls configuration: after the method name and its parameters, you can pass `true` and done, you just declared a wither:
```yaml
services:
    MyService:
        calls:
            - [withLogger, ['@logger'], true]
```

In XML, you could use the new `returns-clone` attribute on the `<call>` tag.

And when using autowiring, the code looks for the `@return static` annotation and turns the flag on if found.

There is only one limitation: unlike services with regular setters, services with withers cannot be part of circular loops that involve calls to wither methods (unless they're declared lazy of course).

Commits
-------

f455d1b [DI] Add support for "wither" methods - for greater immutable services

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:di-withers branch Apr 3, 2019

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.