Skip to content

[DI] Allow extensions to create ServiceLocator as services #21770

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

Merged
merged 1 commit into from
Mar 5, 2017

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Feb 26, 2017

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

https://github.com/symfony/symfony/pull/21770/files?w=1

With this PR, DI extensions are able to create "service locator" services.
They are easily created as such:

$container->register('my_service_locator', ServiceLocator::class)
    ->addArgument(array(
        'exposed_id' => new ServiceClosureArgument(new Reference('internal_id')),
    ))

I already need this in two different PRs to come.

@@ -1127,6 +1132,9 @@ public function resolveServices($value)
$parameterBag = $this->getParameterBag();
$services = array();
foreach ($value->getValues() as $k => $v) {
if ($v->getInvalidBehavior() === ContainerInterface::IGNORE_ON_INVALID_REFERENCE && !$this->has((string) $v)) {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

good catch

@nicolas-grekas nicolas-grekas force-pushed the di-service-locator branch 2 times, most recently from 5af7880 to c831e2d Compare February 28, 2017 17:06
@nicolas-grekas
Copy link
Member Author

@stof I updated this PR so that ServiceLocatorArgument doesn't need to be a Definition anymore. I now use a new ServiceClosureArgument type instead. See updated PR description.
How does it look to you? The PR only needs tests now to me.

return sprintf("function ()%s { return %s; }", $this->dumpValue($reference, $interpolate));
}

return sprintf("function ()%s {\n return %s;\n }", $this->dumpValue($reference, $interpolate));
Copy link
Member

@chalasr chalasr Feb 28, 2017

Choose a reason for hiding this comment

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

not enough values|too much placeholders?

should be return sprintf("function (): %s {\n return %s;\n }", $type, $this->dumpValue($reference, $interpolate)); at the end, same above (missing colon+space after parenthesis in case the type exists, and $type argument)

Copy link
Member Author

Choose a reason for hiding this comment

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

why? indentation is desired in the output
the type is not mandatory, always putting the : would be a parse error

Copy link
Member

Choose a reason for hiding this comment

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

So $type misses a : when available and $type is missing as second argument of sprintf?

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed, fixed

@nicolas-grekas nicolas-grekas force-pushed the di-service-locator branch 5 times, most recently from da4e441 to 4cf0b83 Compare February 28, 2017 22:22
@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Feb 28, 2017

Tests added, PR ready

@nicolas-grekas nicolas-grekas force-pushed the di-service-locator branch 5 times, most recently from db6375f to 54855fe Compare March 2, 2017 13:07
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.

👍

Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

👍

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Mar 4, 2017

I realized that there is a way to decouple autowiring and typed references.
Thus, I renamed the previously called AutowirableReference to TypedReference.
These will be autowired only if their holding-definition is itself autowired.
Independently from autowiring, these references allow to generate return-type-hinted service-locator factories, making things more strict thus more robust for the user.

@fabpot
Copy link
Member

fabpot commented Mar 5, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 1d96633 into symfony:master Mar 5, 2017
fabpot added a commit that referenced this pull request Mar 5, 2017
…ices (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Allow extensions to create ServiceLocator as 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        | -

https://github.com/symfony/symfony/pull/21770/files?w=1

With this PR, DI extensions are able to create "service locator" services.
They are easily created as such:
```php
$container->register('my_service_locator', ServiceLocator::class)
    ->addArgument(array(
        'exposed_id' => new ServiceClosureArgument(new Reference('internal_id')),
    ))
```
I already need this in two different PRs to come.

Commits
-------

1d96633 [DI] Allow creating ServiceLocator-based services in extensions
@nicolas-grekas nicolas-grekas deleted the di-service-locator branch March 5, 2017 21:08
fabpot added a commit that referenced this pull request Mar 22, 2017
…s" tag to inject services into actions (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[FrameworkBundle] Add new "controller.service_arguments" tag to inject services into actions

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | (no test yet)
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Talking with @simensen and @weaverryan, we wondered if we could leverage the `ArgumentResolver` mechanism to make it inject services on demand, using e.g. autowiring.

```php
class PostController
{
  public function indexAction(Request $request, PostRepository $postRepository)
  {
    // PostRepository comes from the container
    $postRepository->findAll(); // ...
  }
}
```

This PR achieves that, using a new "controller.service_arguments" tag. Typically:
```yaml
services:
    AppBundle\Controller\PostController:
        autowire: true
        tags:
            - name: controller.service_arguments
```

It also supports with explicit wiring (thus doesn't necessarily require autowiring if you don't want to use it):
```yaml
services:
    AppBundle\Controller\PostController:
        tags:
            - name: controller.service_arguments
              action: fooAction
              argument: logger
              id: my_logger
```

~~The attached diff is bigger than strictly required for now, until #21770 is merged.~~

Todo:
- [x] rebase on top of #21770 when merged
- [x] add tests
- [x] add cleaning pass to remove empty service locators

Commits
-------

9c6e672 [FrameworkBundle] Add new "controller.service_arguments" tag to inject services into actions
@fabpot fabpot mentioned this pull request May 1, 2017
@TomasVotruba
Copy link
Contributor

TomasVotruba commented May 1, 2017

I got feeling that I might use this feature... but do not understand it yet.

Could you share some more "before/after" examples please? 2-3 would be great, so I could understand it better.

@nicolas-grekas
Copy link
Member Author

Can't answer in detail now, but please check #21708 it might help, either the code or the feature.

OskarStark added a commit to symfony/symfony-docs that referenced this pull request Mar 9, 2023
…es (HypeMC)

This PR was merged into the 5.4 branch.

Discussion
----------

[DependencyInjection] Add section about Service Closures

Documents service closures added in symfony/symfony#21770 and symfony/symfony#41176, and the `service_closure()` PHP-DSL function added in symfony/symfony#42625 .

Commits
-------

f2b2fdb [DI] Add section about Service Closures
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants