More clear description of factory service creation #7377
Conversation
| 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.
The double colon at the end of this line should be a single colon.
|
|
||
| class NewsletterManagerFactory | ||
| class NewsletterManagerStaticFactory |
There was a problem hiding this comment.
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.
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.
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.
(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...
| @@ -13,9 +13,9 @@ the service container to call a method on the factory rather than directly | |||
| instantiating the class. | |||
|
|
|||
| Suppose you have a factory that configures and returns a new ``NewsletterManager`` | |||
| object:: | |||
| object, by calling the static ``createNewsletterManager()`` method:: | |||
|
|
||
| .. code-block:: xml | ||
|
|
There was a problem hiding this comment.
we should add a comment containing the filename like this:
<!-- app/config/services.xml -->There was a problem hiding this comment.
Can you please add this for the other formats too (using config.yml and config.php respectively)?
There was a problem hiding this comment.
I've done this. Am however wondering whether it's not app/config/services.yml and app/config/services.php to be exactly?
There was a problem hiding this comment.
Of course, you are absolutely right. Sorry for the confusion.
|
Fixed everything mentioned. Shall I squash everything into one commit? |
|
@hvt If you want to, you can do that. Otherwise, we'll do that while merging your PR. :) |
|
Done :]. |
|
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 creation 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... Commits ------- a5f99d8 More clear description of factory service creation
|
@xabbuh, will it be merged into master and all >2.7 branches too? |
|
@hvt Yes, I'll do that later today. Thank you for the heads up. |
|
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...