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

[WIP] Static injection in DIC #10991

Open
wants to merge 34 commits into
base: master
from

Conversation

@Guikingone
Copy link
Contributor

commented Feb 12, 2019

Hi everyone,

As discussed with @nicolas-grekas, the DIC is planned to be capable of ensuring services immutability (using "wither" calls), the implementation can be found here:

feat(DI): static injection
Static injection description started

Linked to symfony/symfony@master...nicolas-grekas:di-withers

@javiereguiluz javiereguiluz changed the title [DIC] Static injection [WIP] Static injection in DIC Feb 12, 2019

@javiereguiluz javiereguiluz added this to the next milestone Feb 12, 2019

Guikingone added some commits Feb 12, 2019

feat(DIC): start on static usage
The static usage has been started, updates to come.
@Guikingone

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

Symfony PR added.

fix(code): fix on both code & service conf
Small fix on code and service configuration.
fix(code): fix on constructor argument
Co-Authored-By: Guikingone <guillaume.loulier@hotmail.fr>
@zanbaldwin

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

Do you think it would be useful to add another wither method (eg, withLogger) to demonstrate the clone $this way showcased in symfony/symfony#30212 (in addition to the new static way mentioned here) - or would that be too much information and make the docs look cluttered?

@Guikingone

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

Actually, we can demonstrate both, IMHO, if we showcase both, we need to add a new service (maybe like the one in the PR?) in order to ease the readability.

@nicolas-grekas
Copy link
Member

left a comment

thanks, is this the only page that needs to be updated? do we talk about setters anywhere else?

Show resolved Hide resolved service_container/injection_types.rst Outdated
/**
*
* The @return is required.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 13, 2019

Member

I wouldn't add this to the code block. If it needs to be somewhere, that's in the doc itself.
Instead, we could replace it with @required + link to the doc page that tells what this does.

Show resolved Hide resolved service_container/injection_types.rst Outdated
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Here is a page that describes something similar, could be useful for inspiration and wordings.
https://projectlombok.org/features/experimental/Wither

@Guikingone

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

Maybe using things like Immutable injection or even Immutable service could be a solution? 🤔

Guikingone added some commits Mar 18, 2019

Show resolved Hide resolved service_container/injection_types.rst
Show resolved Hide resolved service_container/calls.rst
Show resolved Hide resolved service_container/injection_types.rst
@@ -71,3 +71,74 @@ To configure the container to call the ``setLogger`` method, use the ``calls`` k
$container->register(MessageGenerator::class)
->addMethodCall('setLogger', [new Reference('logger')]);

This comment has been minimized.

Copy link
@OskarStark

OskarStark Mar 30, 2019

Contributor

Maybe adding some kind of headline here to have a better structured page? 🤔

This comment has been minimized.

Copy link
@Guikingone

Guikingone Apr 22, 2019

Author Contributor

Can you explain what you're thinking about? I'm not sure to understand it clearly 😄 🤔


* As this approach force the container to create a new object
once the method is called, you can found hard to debug and test your code.

This comment has been minimized.

Copy link
@OskarStark

OskarStark Mar 30, 2019

Contributor

The whole new docs part should be below the setter injection part, what do you think?

This comment has been minimized.

Copy link
@Guikingone

Guikingone Apr 3, 2019

Author Contributor

It depends, @nicolas-grekas can probably answer this question better than me, does the "immutable-setter" injection is recommended over the setter one?

Show resolved Hide resolved service_container/calls.rst Outdated
Show resolved Hide resolved service_container/calls.rst Outdated

symfony-splitter pushed a commit to symfony/dependency-injection 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
-------

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

symfony-splitter pushed a commit to symfony/framework-bundle 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
-------

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

fabpot added a commit to symfony/symfony 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
refactor(core): refactoring on multiples suggestions
Fix on ticks

Fix on method usage

MailerInterface namespace added

@Guikingone Guikingone force-pushed the Guikingone:patch-21 branch from b1fdb44 to 577760f Apr 3, 2019

refactor(injection_types): refactoring on disadvantages
Refactoring on disadvantages for setter and immutable setter.

@Guikingone Guikingone force-pushed the Guikingone:patch-21 branch from 8c81e56 to af46b4f Apr 3, 2019

<container xmlns="http://symfony.com/schema/dic/services"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://symfony.com/schema/dic/services
http://symfony.com/schema/dic/services/services-1.0.xsd">

This comment has been minimized.

Copy link
@OskarStark

OskarStark Apr 3, 2019

Contributor

https please

This comment has been minimized.

Copy link
@Guikingone

Guikingone Apr 3, 2019

Author Contributor

Added :)

Show resolved Hide resolved service_container/injection_types.rst
refactor(DI): https added
HTTPS added for both Symfony & W3C
@Guikingone

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

@nicolas-grekas See this post this morning : https://symfony.com/blog/new-in-symfony-4-3-configuring-services-with-immutable-setters.

Does the @return static annotation is now considered as required? 🤔

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

It's required anyway if you want to properly document your return types, isn't it?

@Guikingone

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

@nicolas-grekas For sure, I'm gonna fix the latest reviews and submit a final version of this proposal this week, can you review it when finished? 🤔

@javiereguiluz javiereguiluz modified the milestones: next, 4.3 Apr 5, 2019

@nicolas-grekas
Copy link
Member

left a comment

some more review feedback

Show resolved Hide resolved service_container/calls.rst Outdated
Show resolved Hide resolved service_container/injection_types.rst

@Guikingone Guikingone force-pushed the Guikingone:patch-21 branch from 554c50c to b626ac8 Apr 19, 2019

Guikingone added some commits Apr 22, 2019

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.