Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
More clear description of factory service creation#7377
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
javiereguiluz left a comment
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.
👍
service_container/factories.rst Outdated
| service too. Later, in the ":ref:`factories-passing-arguments-factory-method`" | ||
| section, you learn how you can inject arguments in this method. | ||
| Configuration of the service container then looks like this:: |
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 double colon at the end of this line should be a single colon.
| object, by calling the static ``createNewsletterManager()`` method:: | ||
| classNewsletterManagerFactory | ||
| classNewsletterManagerStaticFactory |
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 class name looks ugly ... but at the same time it's perfectly understandable. I'm divided about it 😕
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.
Exactly my thought, I'm happy to change it either way ;].
| When using a factory to create services, the value chosen for the ``class`` | ||
| option has no effect on the resulting service. The actual class name | ||
| only depends on the object that is returned by the factory. However, |
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.
About this:
However, the configured class name may be used by compiler passes and therefore should be set to a sensible value.
Should I open a Symfony code issue to improve this behavior and make this class optional?
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 note was already there, don't know why it is marked as new by the diff.)
I do however believe the note is still valid, because it might be handy to use the class definition somewhere in a compiler pass.
On the other hand, it might be good to check why no active validation is done on the return type of the creation method...
service_container/factories.rst Outdated
| Suppose you have a factory that configures and returns a new ``NewsletterManager`` | ||
| object:: | ||
| object, by calling the static ``createNewsletterManager()`` method:: |
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 comma should be removed
| factory:['AppBundle\Email\NewsletterManagerStaticFactory',createNewsletterManager] | ||
| ..code-block::xml | ||
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 add a comment containing the filename like this:
<!-- app/config/services.xml-->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.
Can you please add this for the other formats too (usingconfig.yml andconfig.php respectively)?
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've done this. Am however wondering whether it's notapp/config/services.yml andapp/config/services.php to be exactly?
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.
Of course, you are absolutely right. Sorry for the confusion.
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.
No worries ;].
hvt commentedJan 18, 2017
Fixed everything mentioned. Shall I squash everything into one commit? |
xabbuh commentedJan 18, 2017
@hvt If you want to, you can do that. Otherwise, we'll do that while merging your PR. :) |
hvt commentedJan 18, 2017
Done :]. |
xabbuh commentedJan 31, 2017
Thank you@hvt. |
This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes#7377).Discussion----------More clear description of factory service creationI found the service container factory description a bit ambiguous. Especially the two different examples of both static and regular create method usage. Hopefully this clarifies some things.Eager to know what you guys think...Commits-------a5f99d8 More clear description of factory service creation
hvt commentedFeb 1, 2017
@xabbuh, will it be merged into master and all >2.7 branches too? |
xabbuh commentedFeb 1, 2017
@hvt Yes, I'll do that later today. Thank you for the heads up. |
hvt commentedAug 9, 2017
Hey@xabbuh , found this in the bottom of my inbox ;]. |
I found the service container factory description a bit ambiguous. Especially the two different examples of both static and regular create method usage. Hopefully this clarifies some things.
Eager to know what you guys think...