[DI] Optional class for named services #21133

Merged
merged 2 commits into from Jan 7, 2017

Conversation

@nicolas-grekas
Member

nicolas-grekas commented Jan 2, 2017

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

Continues #20264:

  • makes the id-to-class mapping context-free (no class_exist check),
  • deals with ChildDefinition (which should not have this rule applied on them),
  • deprecates FactoryReturnTypePass as discussed on slack and reported in this comment: #19191 (comment)

From @hason:

I prefer class named services (in applications) because naming things are too hard:

services:
    Vendor\Namespace\Class:
        class: Vendor\Namespace\Class
        autowire: true

This PR solves redundant parameter for class:

services:
    Vendor\Namespace\Class:
        autowire: true

Inspirations: https://laravel.com/docs/5.3/container, #18268, http://php-di.org/,

@@ -29,6 +42,9 @@ public function process(ContainerBuilder $container)
if (!method_exists(\ReflectionMethod::class, 'getReturnType')) {
return;
}
+ if (null !== $this->resolveClassPass) {
+ $this->resolveClassPassChanges = $this->resolveClassPass->getChanges();

This comment has been minimized.

@stof

stof Jan 2, 2017

Member

this property should be reset at the end of processing to release memory

@stof

stof Jan 2, 2017

Member

this property should be reset at the end of processing to release memory

This comment has been minimized.

This comment has been minimized.

@xabbuh

xabbuh Jan 3, 2017

Member

Did you forget to push the change or do I miss something? I don't see where it is reset.

@xabbuh

xabbuh Jan 3, 2017

Member

Did you forget to push the change or do I miss something? I don't see where it is reset.

This comment has been minimized.

@xabbuh

xabbuh Jan 3, 2017

Member

Saw it now.

@xabbuh

xabbuh Jan 3, 2017

Member

Saw it now.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 3, 2017

Member

in fact, I removed the property completely for a local variable instead

@nicolas-grekas

nicolas-grekas Jan 3, 2017

Member

in fact, I removed the property completely for a local variable instead

+ *
+ * @return string
+ */
+ public function getCaseFullId($id)

This comment has been minimized.

@GuilhemN

GuilhemN Jan 2, 2017

Contributor

What about getRawId() or getOriginalId()?

@GuilhemN

GuilhemN Jan 2, 2017

Contributor

What about getRawId() or getOriginalId()?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 2, 2017

Member

bof :)

This comment has been minimized.

@GuilhemN

GuilhemN Jan 2, 2017

Contributor

But does case full id really exists in english ?

@GuilhemN

GuilhemN Jan 2, 2017

Contributor

But does case full id really exists in english ?

This comment has been minimized.

@wouterj

wouterj Jan 2, 2017

Member

Yeah, if anything it should be casefullId (or getCasefullId). But I like @theofidry's caseSensitiveId more 👍

@wouterj

wouterj Jan 2, 2017

Member

Yeah, if anything it should be casefullId (or getCasefullId). But I like @theofidry's caseSensitiveId more 👍

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 3, 2017

Member

updated to caseSensitive

@nicolas-grekas

nicolas-grekas Jan 3, 2017

Member

updated to caseSensitive

if ($this->isFrozen() && (isset($this->definitions[$id]) && !$this->definitions[$id]->isSynthetic())) {
// setting a synthetic service on a frozen container is alright
throw new BadMethodCallException(sprintf('Setting service "%s" for an unknown or non-synthetic service definition on a frozen container is not allowed.', $id));
}
unset($this->definitions[$id], $this->aliasDefinitions[$id]);
+ if ($id !== $caseFullId) {
+ $this->caseFullIds[$id] = $caseFullId;

This comment has been minimized.

@theofidry

theofidry Jan 2, 2017

Contributor

caseSensitiveIds?

@theofidry

theofidry Jan 2, 2017

Contributor

caseSensitiveIds?

This comment has been minimized.

@xabbuh

xabbuh Jan 3, 2017

Member

👍 seems more obvious what is meant

@xabbuh

xabbuh Jan 3, 2017

Member

👍 seems more obvious what is meant

@dunglas

dunglas approved these changes Jan 3, 2017

@@ -101,6 +101,11 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
private $envCounters = array();
+ /**
+ * @var string[] A map of case less to case full ids

This comment has been minimized.

@xabbuh

xabbuh Jan 3, 2017

Member

string[] does not cover arrays where keys are no integers, does it?

@xabbuh

xabbuh Jan 3, 2017

Member

string[] does not cover arrays where keys are no integers, does it?

@@ -837,6 +854,20 @@ public function findDefinition($id)
}
+ /**
+ * Returns the case full id used at registration time if any.

This comment has been minimized.

@xabbuh

xabbuh Jan 3, 2017

Member

I would remove the "if any" part because we always return the id that was used when defining the definition. This makes it look likenull might be returned in some cases too.

@xabbuh

xabbuh Jan 3, 2017

Member

I would remove the "if any" part because we always return the id that was used when defining the definition. This makes it look likenull might be returned in some cases too.

@nicolas-grekas nicolas-grekas changed the title from [DI] Optional class for class named services to [DI] Optional class for named services Jan 3, 2017

@@ -301,7 +301,7 @@ private function processAnonymousServices(\DOMDocument $xml, $file)
if (false !== $nodes = $xpath->query('//container:argument[@type="service"][not(@id)]|//container:property[@type="service"][not(@id)]')) {
foreach ($nodes as $node) {
// give it a unique name
- $id = sprintf('%s_%d', hash('sha256', $file), ++$count);
+ $id = sprintf('%d_%s', ++$count, hash('sha256', $file));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 3, 2017

Member

just made these changes so that anonymous classes won't have their class set to a random string (+ added corresponding regexp check in ResolveClassPass)

@nicolas-grekas

nicolas-grekas Jan 3, 2017

Member

just made these changes so that anonymous classes won't have their class set to a random string (+ added corresponding regexp check in ResolveClassPass)

This comment has been minimized.

@dunglas

dunglas Jan 3, 2017

Member

Why hashing it? Wouldn't it be more explicit with the plain file name?

@dunglas

dunglas Jan 3, 2017

Member

Why hashing it? Wouldn't it be more explicit with the plain file name?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 3, 2017

Member

not related to this PR :)

@nicolas-grekas

nicolas-grekas Jan 3, 2017

Member

not related to this PR :)

+ if ($definition instanceof ChildDefinition || $definition->isSynthetic() || null !== $definition->getClass()) {
+ continue;
+ }
+ if (preg_match('/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/', $id)) {

This comment has been minimized.

@OskarStark

OskarStark Jan 3, 2017

Contributor

shouldn't this be covered by a test?

@OskarStark

OskarStark Jan 3, 2017

Contributor

shouldn't this be covered by a test?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 3, 2017

Member

covered now

@nicolas-grekas

nicolas-grekas Jan 3, 2017

Member

covered now

This comment has been minimized.

@OskarStark

OskarStark Jan 4, 2017

Contributor

thank you 👍

@OskarStark

OskarStark Jan 4, 2017

Contributor

thank you 👍

@@ -638,6 +648,9 @@ public function setAlias($alias, $id)
}
unset($this->definitions[$alias]);
+ if ($alias !== $caseSensitiveAlias) {
+ $this->caseSensitiveIds[$alias] = $caseSensitiveAlias;

This comment has been minimized.

@xabbuh

xabbuh Jan 3, 2017

Member

Do we actually need the entry for aliases or could we just remove existing entries here?

@xabbuh

xabbuh Jan 3, 2017

Member

Do we actually need the entry for aliases or could we just remove existing entries here?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 3, 2017

Member

For completeness yes: we track any id, wherever it comes from

@nicolas-grekas

nicolas-grekas Jan 3, 2017

Member

For completeness yes: we track any id, wherever it comes from

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Jan 3, 2017

Member

👍

Status: Reviewed

Member

xabbuh commented Jan 3, 2017

👍

Status: Reviewed

@@ -364,14 +369,18 @@ public function getCompiler()
*/
public function set($id, $service)
{
- $id = strtolower($id);
+ $caseSensitiveId = (string) $id;

This comment has been minimized.

@fabpot

fabpot Jan 7, 2017

Member

Why do you add a cast here?

@fabpot

fabpot Jan 7, 2017

Member

Why do you add a cast here?

This comment has been minimized.

@wouterj

wouterj Jan 7, 2017

Member

Probably to be BC with the previous behaviour: It was possible to pass e.g. an object with __toString() as $id as strtolower() cast everything to a string.

@wouterj

wouterj Jan 7, 2017

Member

Probably to be BC with the previous behaviour: It was possible to pass e.g. an object with __toString() as $id as strtolower() cast everything to a string.

This comment has been minimized.

@fabpot

fabpot Jan 7, 2017

Member

If that's the reason, I would revert this as $id is documented as being a string.

@fabpot

fabpot Jan 7, 2017

Member

If that's the reason, I would revert this as $id is documented as being a string.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 7, 2017

Member

reverted

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jan 7, 2017

Member

Thank you @nicolas-grekas.

Member

fabpot commented Jan 7, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit a18c4b6 into symfony:master Jan 7, 2017

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 Jan 7, 2017

feature #21133 [DI] Optional class for named services (hason, nicolas…
…-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Optional class for named services

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

Continues #20264:
- makes the id-to-class mapping context-free (no `class_exist` check),
- deals with ChildDefinition (which should not have this rule applied on them),
- deprecates FactoryReturnTypePass as discussed on slack and reported in this comment: #19191 (comment)

From @hason:

> I prefer class named services (in applications) because naming things are too hard:

``` yaml
services:
    Vendor\Namespace\Class:
        class: Vendor\Namespace\Class
        autowire: true
```

> This PR solves redundant parameter for class:

``` yaml
services:
    Vendor\Namespace\Class:
        autowire: true
```

> Inspirations: https://laravel.com/docs/5.3/container, #18268, http://php-di.org/,

Commits
-------

a18c4b6 [DI] Add tests for class named services
71b17c7 [DI] Optional class for named services

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:class-service branch Jan 7, 2017

@wouterj wouterj referenced this pull request in symfony/symfony-docs Jan 7, 2017

Closed

[3.3][DI] Service ids now default to the classname #7329

@wouterj

This comment has been minimized.

Show comment
Hide comment
@wouterj

wouterj Jan 7, 2017

Member

Fyi, created a doc issue for this great feature: symfony/symfony-docs#7329

Member

wouterj commented Jan 7, 2017

Fyi, created a doc issue for this great feature: symfony/symfony-docs#7329

fabpot added a commit that referenced this pull request Jan 8, 2017

bug #21203 [DependencyInjection] moved up ResolveClassPass in the con…
…tainer pass list (fabpot)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DependencyInjection] moved up ResolveClassPass in the container pass list

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | bug from #21133
| License       | MIT
| Doc PR        | n/a

Some compiler passes need access to the service class names. But when using an empty class name (the service id being the class name), the resolution happens too late (during the optimization step). This PR fixes this by moving up the ResolveClassPass earlier in the stack.

Commits
-------

2e5b69f [DependencyInjection] moved up ResolveClassPass in the container pass list

@Haehnchen Haehnchen referenced this pull request in Haehnchen/idea-php-symfony2-plugin Jan 9, 2017

Closed

[DI] Support optional class for named services #847

fabpot added a commit that referenced this pull request Jan 23, 2017

feature #21313 [DI] Add Yaml syntax for short services definition (og…
…izanagi)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Add Yaml syntax for short services definition

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

In my experience, most (at least, a lot) of the service definitions in an end-application only require the class and constructor arguments.

#21133 allows to get rid of the `class` attribute by using the service id.
Which means only arguments remain for most use-cases. Hence, we could support this syntax:

#### Before:

```yml
services:
    App\Foo\Bar:
        arguments: ['@baz', 'foo', '%qux%']
```

#### After:

```yml
services:
    App\Foo\Bar: ['@baz', 'foo', '%qux%']
```

It works especially well along with services `_defaults` introduced in #21071 :

```yml
services:
    _defaults:
        public: false
        autowire: ['set*']
        tags: ['serializer.normalizer']

    App\Serializer\FooNormalizer: ['@baz', 'foo', '%qux%']
```

Commits
-------

83b599c [DI] Add Yaml syntax for short services definition

@fabpot fabpot referenced this pull request May 1, 2017

Merged

Release v3.3.0-BETA1 #22603

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