Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Optional class for named services#21133
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
618d432 to80e4aaeCompare| return; | ||
| } | ||
| if (null !==$this->resolveClassPass) { | ||
| $this->resolveClassPassChanges =$this->resolveClassPass->getChanges(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this property should be reset at the end of processing to release memory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Did you forget to push the change or do I miss something? I don't see where it is reset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Saw it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
in fact, I removed the property completely for a local variable instead
af7dfda to5def4a7Compare| * @param string $id | ||
| * | ||
| * @return string | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What aboutgetRawId() orgetOriginalId()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
bof :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But doescase full id really exists in english ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, if anything it should becasefullId (orgetCasefullId). But I like@theofidry'scaseSensitiveId more 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
updated to caseSensitive
| unset($this->definitions[$id],$this->aliasDefinitions[$id]); | ||
| if ($id !==$caseFullId) { | ||
| $this->caseFullIds[$id] =$caseFullId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
caseSensitiveIds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 seems more obvious what is meant
| */ | ||
| private$envCounters =array(); | ||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
string[] does not cover arrays where keys are no integers, does it?
| return$this->getDefinition($id); | ||
| } | ||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
f18bbe7 to1d766abCompare4b7cd67 to01afe39Compare| foreach ($nodesas$node) { | ||
| // give it a unique name | ||
| $id =sprintf('%s_%d',hash('sha256',$file), ++$count); | ||
| $id =sprintf('%d_%s',++$count,hash('sha256',$file)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
just made these changes so that anonymous classes won't have their class set to a random string (+ added corresponding regexp check in ResolveClassPass)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why hashing it? Wouldn't it be more explicit with the plain file name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not related to this PR :)
| if ($definitioninstanceof ChildDefinition ||$definition->isSynthetic() ||null !==$definition->getClass()) { | ||
| continue; | ||
| } | ||
| if (preg_match('/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/',$id)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
shouldn't this be covered by a test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
covered now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
thank you 👍
68e9588 to405b151Compare| unset($this->definitions[$alias]); | ||
| if ($alias !==$caseSensitiveAlias) { | ||
| $this->caseSensitiveIds[$alias] =$caseSensitiveAlias; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we actually need the entry for aliases or could we just remove existing entries here?
nicolas-grekasJan 3, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For completeness yes: we track any id, wherever it comes from
xabbuh commentedJan 3, 2017
👍 Status: Reviewed |
| publicfunctionset($id,$service) | ||
| { | ||
| $id =strtolower($id); | ||
| $caseSensitiveId = (string)$id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why do you add a cast here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Probably to be BC with the previous behaviour: It was possible to pass e.g. an object with__toString() as$id asstrtolower() cast everything to a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If that's the reason, I would revert this as$id is documented as being a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
reverted
405b151 toa18c4b6Comparefabpot commentedJan 7, 2017
Thank you@nicolas-grekas. |
…-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:``` yamlservices: Vendor\Namespace\Class: class: Vendor\Namespace\Class autowire: true```> This PR solves redundant parameter for class:``` yamlservices: 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 services71b17c7 [DI] Optional class for named services
wouterj commentedJan 7, 2017
Fyi, created a doc issue for this great feature:symfony/symfony-docs#7329 |
…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/aSome 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
…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 | todoIn 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:```ymlservices: App\Foo\Bar: arguments: ['@baz', 'foo', '%qux%']```#### After:```ymlservices: App\Foo\Bar: ['@baz', 'foo', '%qux%']```It works especially well along with services `_defaults` introduced in#21071 :```ymlservices: _defaults: public: false autowire: ['set*'] tags: ['serializer.normalizer'] App\Serializer\FooNormalizer: ['@baz', 'foo', '%qux%']```Commits-------83b599c [DI] Add Yaml syntax for short services definition
Uh oh!
There was an error while loading.Please reload this page.
Continues#20264:
class_existcheck),From@hason: