Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
[DI] Add section about getter injection#7300
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
| The disadvantages of getter injection are: | ||
| * You must call the getter everytime you need the dependency internally. |
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 isn't any property holding the dependency to use anyway. Is it worth mentioning this as a drawback? 😅
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.
That's actually the same drawback as outlined here, reformulated :)
ogizanagiDec 29, 2016 • 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.
I meant this should probably not appear in the drawbacks section at all. It isn't one. Just a consequence of the above. Which has no impact.
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.
oh! removed then!
| app.newsletter_manager: | ||
| class:AppBundle\Mail\NewsletterManager | ||
| getters: | ||
| -getMailer:'@mailer' |
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 it rather be:
app.newsletter_manager:class:AppBundle\Mail\NewsletterManagergetters:getMailer:'@mailer'getLogger:'@logger'
?
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
f9144d5 tof8d3457Compare| // ... | ||
| abstract class NewsletterManager | ||
| { | ||
| abstract protected function getMailer(): MailerInterface; |
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 these return types are mandatory, we must add somewhere that this only works with PHP 7. Otheriwse, we should remove them.
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 forgot to mention that PHP 7 is required when using getter injection. Added now.
About the type hints, they are not required, but highly recommended. Given the PHP 7 requirement, I think we should keep them here.
f8d3457 to801ea33Compare801ea33 to2d32d66Compare| ---------------- | ||
| ..versionadded::3.3 | ||
| Getter Injection was introduced in Symfony 3.3. |
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.
We should mention that this is an experimental feature
| Getter Injection | ||
| ---------------- | ||
| ..versionadded::3.3 |
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.
The leading spaces should be removed.
This PR was merged into the 3.3-dev branch.Discussion----------[DI] Add getter injection| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#20657| License | MIT| Doc PR |symfony/symfony-docs#7300Getter overriding by the container will allow a new kind of dependency injection which enables easier laziness and more immutable classes, by not requiring any corresponding setter. See linked issue for more.This is WIP:- [x] wire the concept- [x] dump anonymous classes with PhpDumper- [x] generate at runtime in ContainerBuilder::createService- [x] tests- [x] make it work on PHP 5Commits-------cb49858 [DI] Add getter injection
This PR was merged into the 3.3-dev branch.Discussion----------[DI] Add getter injection| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | #20657| License | MIT| Doc PR |symfony/symfony-docs#7300Getter overriding by the container will allow a new kind of dependency injection which enables easier laziness and more immutable classes, by not requiring any corresponding setter. See linked issue for more.This is WIP:- [x] wire the concept- [x] dump anonymous classes with PhpDumper- [x] generate at runtime in ContainerBuilder::createService- [x] tests- [x] make it work on PHP 5Commits-------cb498580d1 [DI] Add getter injection
| // ... | ||
| $container->register('app.newsletter_manager', NewsletterManager::class) | ||
| ->addOverriddenGetter('getMailer', new Reference('mailer')) |
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.
Should besetOverriddenGetter (same below).
nicolas-grekas commentedMar 25, 2017
Reverted, one less thing to document for 3.3 :) |
Uh oh!
There was an error while loading.Please reload this page.
Refsymfony/symfony#20973