First, I want to thank you all for taking the time to comment the first RFC. After the discussions that took place, I have created a version 2 of the RFC:
It hopefully takes into account all the feedback we had. Some personal feelings after writing this second version of the RFC:
* Testability is not a valid concern: As the container implements the ArrayAccess interface (to access parameters) and the `__get` and `__set` methods (to access the services), creating one for testing is super-easy.
* Lots of people agree that passing dependencies as arguments (constructor or action methods) is the way to go but these approaches involve some magic.
* There is no "best approach". Each has some pros and cons. We need to find the approach that fits best Symfony 2 philosophy.
Some approaches have not been included in the RFC for the following reasons:
* Approaches that need code generation;
* Approaches that does not feel like PHP code;
* Approaches that involves too much overhead.
Based on the naming convention of the Dependency Injector issue raised by Jordi, I will write another RFC on that topic.
Now, let's resume the discussion on this new version of the RFC!
In the RFC you imply that the common case is that dependencies will not be used. I think the common case we should expect is that all dependencies will be used. The rare situations that I see in my current work is stuff like mailer or in some cases handles to external data sources which might be made superfluous due to caching. The caching case can only really be handled by injecting the service container I guess.
Approach 3: I think approach 3 has the least merit. It defaults to a lot of magic and is going to be quite unreadable. I also do not understand why you say that "All the magic is optional."? It also doesnt feel like "Very concise code;" to me. Sure you do not need the boilerplate code in the constructor, however you need to repeat the same parameters in all your action methods for all the share dependencies.
Approach 5: I do not have experience with annotations, they do have some appeal and I can see improved support for them in future PHP versions, but I also agree that its a bit tricky to start defining sort of our own phpdoc language. Not really the way to go to make symfony easier to learn.
Approach 1: I do not like approach 1 because of the circular references and the fact that its not so easy to see the dependencies by just looking at the constructor (and action method) signatures.
Approach 2/4 As such I like approach 2 and 4 the best. For approach 4 I would encourage a postfix for services. It can even be something long like "Service". This indirectly encourages people to prefer injecting into the constructor, which imho is the better approach. Once we have traits (I am very certain we will see this in the next bigger version of PHP), we might be able to provide a trait to reduce the boiler plate code.
> In the RFC you imply that the common case is that dependencies will not be used. I think the common case we should expect is that all dependencies will be used. The rare situations that I see in my current work is stuff like mailer or in some cases handles to external data sources which might be made superfluous due to caching. The caching case can only really be handled by injecting the service container I guess.
> Approach 3: > I think approach 3 has the least merit. It defaults to a lot of magic and is going to be quite unreadable. I also do not understand why you say that "All the magic is optional."? It also doesnt feel like "Very concise code;" to me. Sure you do not need the boilerplate code in the constructor, however you need to repeat the same parameters in all your action methods for all the share dependencies.
The magic is optional as you are not forced to pass any service in your methods. You are right about conciseness. The method code is more concise, but the method signature is not anymore.
> Approach 5: > I do not have experience with annotations, they do have some appeal and I can see improved support for them in future PHP versions, but I also agree that its a bit tricky to start defining sort of our own phpdoc language. Not really the way to go to make symfony easier to learn.
> Approach 1: > I do not like approach 1 because of the circular references and the fact that its not so easy to see the dependencies by just looking at the constructor (and action method) signatures.
Which circular reference? I don't see any.
> Approach 2/4 > As such I like approach 2 and 4 the best. For approach 4 I would encourage a postfix for services. It can even be something long like "Service". This indirectly encourages people to prefer injecting into the constructor, which imho is the better approach. Once we have traits (I am very certain we will see this in the next bigger version of PHP), we might be able to provide a trait to reduce the boiler plate code.
A Service postfix for services sounds good to me. But then, I think we need to be consistent and add the postfix for both services injected in the constructor and those injected in the methods.
> On 3/22/10 1:09 PM, Lukas Kahwe Smith wrote: >> Approach 3: >> I think approach 3 has the least merit. It defaults to a lot of magic and is going to be quite unreadable. I also do not understand why you say that "All the magic is optional."? It also doesnt feel like "Very concise code;" to me. Sure you do not need the boilerplate code in the constructor, however you need to repeat the same parameters in all your action methods for all the share dependencies.
> The magic is optional as you are not forced to pass any service in your methods. You are right about conciseness. The method code is more concise, but the method signature is not anymore.
Hmm, how is it optional? With approach 3 you do not have another way to inject, unless you are referring to injecting the service container, which still means that there is no way around any kind of magic.
>> Approach 1: >> I do not like approach 1 because of the circular references and the fact that its not so easy to see the dependencies by just looking at the constructor (and action method) signatures.
> Which circular reference? I don't see any.
Isnt the controller also handled by the service container?
>> Approach 2/4 >> As such I like approach 2 and 4 the best. For approach 4 I would encourage a postfix for services. It can even be something long like "Service". This indirectly encourages people to prefer injecting into the constructor, which imho is the better approach. Once we have traits (I am very certain we will see this in the next bigger version of PHP), we might be able to provide a trait to reduce the boiler plate code.
> A Service postfix for services sounds good to me. But then, I think we need to be consistent and add the postfix for both services injected in the constructor and those injected in the methods.
Well ... my point was that a postfix, especially a long one, makes it abundantly clear which one is the preferred method. Plus I do not think its all that inconsistent. There are no collisions possible in the constructor, but there are in the method signature.
--
Of course we could also just not inject the route parameters so that we do not have naming collisions if we go with approach 3 or 4. But I would find that quite unfortunate.
> On 22.03.2010, at 13:18, Fabien Potencier wrote:
>> On 3/22/10 1:09 PM, Lukas Kahwe Smith wrote: >>> Approach 3: >>> I think approach 3 has the least merit. It defaults to a lot of magic and is going to be quite unreadable. I also do not understand why you say that "All the magic is optional."? It also doesnt feel like "Very concise code;" to me. Sure you do not need the boilerplate code in the constructor, however you need to repeat the same parameters in all your action methods for all the share dependencies.
>> The magic is optional as you are not forced to pass any service in your methods. You are right about conciseness. The method code is more concise, but the method signature is not anymore.
> Hmm, how is it optional? With approach 3 you do not have another way to inject, unless you are referring to injecting the service container, which still means that there is no way around any kind of magic.
You can just inject the container and get the services from that.
>>> Approach 1: >>> I do not like approach 1 because of the circular references and the fact that its not so easy to see the dependencies by just looking at the constructor (and action method) signatures.
>> Which circular reference? I don't see any.
> Isnt the controller also handled by the service container?
>>> Approach 2/4 >>> As such I like approach 2 and 4 the best. For approach 4 I would encourage a postfix for services. It can even be something long like "Service". This indirectly encourages people to prefer injecting into the constructor, which imho is the better approach. Once we have traits (I am very certain we will see this in the next bigger version of PHP), we might be able to provide a trait to reduce the boiler plate code.
>> A Service postfix for services sounds good to me. But then, I think we need to be consistent and add the postfix for both services injected in the constructor and those injected in the methods.
> Well ... my point was that a postfix, especially a long one, makes it abundantly clear which one is the preferred method. Plus I do not think its all that inconsistent. There are no collisions possible in the constructor, but there are in the method signature.
> --
> Of course we could also just not inject the route parameters so that we do not have naming collisions if we go with approach 3 or 4. But I would find that quite unfortunate.
> regards, > Lukas Kahwe Smith > m...@pooteeweet.org
Hi, I share Lukas' concerns about approach 3. It is the most magical, and will lead to messy action function signatures and possible name conflicts that not only need to be resolved by the framework, but also by developers. Personally, I would still go with option 1. Its just a small matter of taste. I feel that as somebody who wants to construct an action from somewhere, I (as code) need to do too much. In approach 1 I could just toss in the container. Also all proposals beside 1 have the problem discussed earlier about matching names (variable against service) In approach 1 you can do container->getUserService() Its much easier to refactor than to search for $userService in actions.
But because 2 is compatible with 1, I have no feelings against 2 :)
I want to avoid code that looks like that:
public function editPostAction($id, $title, $year, $month, $day, Service $Service_Pingbackservice, Service $Service_SSOService, Service $Service_mailer){ //.. ugly stuff ...
}
public function previewPostAction($id, $title, $year, $month, $day, $locale, Service $Service_SSO, Service $Service_RSSBuilder){ //.. ugly stuff ...
}
Especially if you have more than one action using a service you have almost saved the lines of code trade off caused by boilerplate code.
> First, I want to thank you all for taking the time to comment the first RFC. After the discussions that took place, I have created a version 2 of the RFC:
>> First, I want to thank you all for taking the time to comment the first RFC. After the discussions that took place, I have created a version 2 of the RFC:
> Now i like "Approach 4". It's universal & good enough for all.
I would also favor 4 since it empowers users with a choice (and nobody seems to really agree on one best way). As I already pointed out though, and I don't know why it was left out of the second RFC, service variables in actions really should be pre-fixed, post-fixed, or anything that differentiates them from route variables, and route variables should be prevented to have a name that will conflict with that defined pre/post-fix, so as to avoid any problems (WTFs) when refactoring code.
This approach btw allows Fabian to still only inject the container in the constructor and then fetch all services by hand when needed.
As for using the pre/post-fix in the constructor variables, I don't see why. The typical use cases I see are those :
- Give me the container and I will fetch everything: function __construct($container) function list($id) function read($id) function comment($id, $mailerService) => clean
- Give me most of my services in constructor, and every now and then an extra one in the action:
function __construct($request, $user, $db) function list($id) function read($id) function comment($id, $mailerService) => there you bloat it a bit in one call, but it's for the sake of performance and should happen rarely
- I like long signatures, give me all in every function:
function __construct() function list($requestService, $userService, $dbService, $id) function read($requestService, $userService, $dbService, $id) function comment($requestService, $userService, $dbService, $mailerService, $id) => bloated but at least you don't have conflicts
If you force the prefix in the constructor you penalize everyone for the rare use case of the (i.e.) mailer. I don't think it's necessary.
The way I see it, it's not uncommon to use 5-6 services minimum in every request, be it in the action itself or in some helper you call, so I don't think people will really want to declare them all every time in their actions, then forward them to their helper methods etc.. So the added verbosity sounds alright in that place.
>>> First, I want to thank you all for taking the time to comment the first RFC. After the discussions that took place, I have created a version 2 of the RFC:
>> Now i like "Approach 4". It's universal& good enough for all.
> I would also favor 4 since it empowers users with a choice (and nobody > seems to really agree on one best way). As I already pointed out though, > and I don't know why it was left out of the second RFC, service > variables in actions really should be pre-fixed, post-fixed, or anything > that differentiates them from route variables, and route variables > should be prevented to have a name that will conflict with that defined > pre/post-fix, so as to avoid any problems (WTFs) when refactoring code.
It is in the new version of the RFC:
"To avoid naming conflicts and simplify the rules a bit, we can probably introduce some naming conventions (like adding a prefix for services)."
> This approach btw allows Fabian to still only inject the container in > the constructor and then fetch all services by hand when needed.
> As for using the pre/post-fix in the constructor variables, I don't see > why. The typical use cases I see are those :
> - Give me the container and I will fetch everything: > function __construct($container) > function list($id) > function read($id) > function comment($id, $mailerService) > => clean
> - Give me most of my services in constructor, and every now and then an > extra one in the action:
> function __construct($request, $user, $db) > function list($id) > function read($id) > function comment($id, $mailerService) > => there you bloat it a bit in one call, but it's for the sake of > performance and should happen rarely
> - I like long signatures, give me all in every function:
> function __construct() > function list($requestService, $userService, $dbService, $id) > function read($requestService, $userService, $dbService, $id) > function comment($requestService, $userService, $dbService, > $mailerService, $id) > => bloated but at least you don't have conflicts
> If you force the prefix in the constructor you penalize everyone for the > rare use case of the (i.e.) mailer. I don't think it's necessary.
Point taken.
If there is a consensus about the Service suffix, I will have update the RFC accordingly.
> The way I see it, it's not uncommon to use 5-6 services minimum in every > request, be it in the action itself or in some helper you call, so I > don't think people will really want to declare them all every time in > their actions, then forward them to their helper methods etc.. So the > added verbosity sounds alright in that place.
> If there is a consensus about the Service suffix, I will have update the > RFC accordingly.
Great, I would favor $_Mailer because of laziness, but I will live with $mailerService, and I definitely agree it's more clear, although I'm sure many people will be confused with the notion of services at first, so either way it has to be well documented.
> On 22.03.2010 16:03, Fabien Potencier wrote: >> It is in the new version of the RFC:
> Ok my bad, sorry for acting like a victim ;)
>> If there is a consensus about the Service suffix, I will have update the >> RFC accordingly.
> Great, I would favor $_Mailer because of laziness, but I will live with > $mailerService, and I definitely agree it's more clear, although I'm > sure many people will be confused with the notion of services at first, > so either way it has to be well documented.
I tend to prefer the Service suffix as this more explicit. The _ as a prefix has already a meaning in many PHP applications (protected variables).
> I tend to prefer the Service suffix as this more explicit. The _ as a > prefix has already a meaning in many PHP applications (protected > variables).
True, I'll just reiterate my $svcMailer (or mailerSvc) proposal as a last attempt at being lazy, but then I'll bow to mailerService :)
Prefixing or suffixing services still leaves one problem unattended: What if DI variables (like $maxPerPage) collide with request parameters? Naming it $maxPerPageService obviously makes no sense.
Also, having a different magic in the constructor ($mailer) and the actions ($mailerService) could be confusing for many developers.
My personal feeling is that the magic in the action methods may lead to problems in big projects with lots of DI configuration. There it can easily happen that services or DI variables collide with request parameters. Unfortunately these collisions may not be obvious.
So, if we take this approach (which is compatible to most other approaches) Symfony should warn the user when such collisions occur.
> Prefixing or suffixing services still leaves one problem unattended: > What if DI variables (like $maxPerPage) collide with request > parameters? Naming it $maxPerPageService obviously makes no sense.
> Also, having a different magic in the constructor ($mailer) and the > actions ($mailerService) could be confusing for many developers.
> My personal feeling is that the magic in the action methods may lead > to problems in big projects with lots of DI configuration. There it > can easily happen that services or DI variables collide with request > parameters. Unfortunately these collisions may not be obvious.
> So, if we take this approach (which is compatible to most other > approaches) Symfony should warn the user when such collisions occur.
Uhm .. I think only object's should be injectable inside a action method if we are going with approach 4. I do not really see a need to inject arrays and scalars inside an action method. You might as well inject those in the constructor.
> Prefixing or suffixing services still leaves one problem unattended: > What if DI variables (like $maxPerPage) collide with request > parameters? Naming it $maxPerPageService obviously makes no sense.
> Also, having a different magic in the constructor ($mailer) and the > actions ($mailerService) could be confusing for many developers.
> My personal feeling is that the magic in the action methods may lead > to problems in big projects with lots of DI configuration. There it > can easily happen that services or DI variables collide with request > parameters. Unfortunately these collisions may not be obvious.
That's why routing variable always have precedence over DIC services/parameters. That way, no change in the DIC can break a controller.
So, the only collision that can happen is when you implement a new controller. And then, the developer will easily spot that he cannot use a specific name in a route because it conflicts with an existing DIC parameter. It can then rename it right away.
So, the process will be the something like this:
1. The developer creates a new route: /something/:maxPerPage
2. The developer creates the related controller:
function someAction($maxPerPage) { // do something }
3. He wants to use the global parameter maxPerPage, so he adds it to the signature:
function someAction($maxPerPage, $maxPerPage) { }
He realizes that it's not possible, so he renames the routing variable:
/something/:max
function someAction($max, $maxPerPage) { }
4. Problem solved
But most of the time, this won't happen. I think it will never happen in fact. The routing parameter names should be quite short (like max) because they are only used in a given context, whereas global parameters tends to have more verbose names (as they can be used in any context). So, the conflicts should be pretty rare.
Of course, for service names, this is a different story, but adding a suffix like Service solves the conflict problem nicely.
>> On 22.03.2010, at 13:18, Fabien Potencier wrote:
>>> On 3/22/10 1:09 PM, Lukas Kahwe Smith wrote:
>>>> Approach 1: >>>> I do not like approach 1 because of the circular references and the fact that its not so easy to see the dependencies by just looking at the constructor (and action method) signatures.
>>> Which circular reference? I don't see any.
>> Isnt the controller also handled by the service container?
> Nope. It is created by a listener.
Ok, that surprised me :)
In okapi2 we decided to have action classes be services as well. This way we get all the power of the service container import capabilities (remember you didnt want it in sfYaml .. now it seems you also added a trimmed down version to the routing config parsing), but also you can easily replace things (like say a bundle that you like depends on a bundle you dont like .. if they would be using services it would be easy to just replace the one you dont like with another implementation. Furthermore it makes handling of scalar/array dependencies easier, since then you can also manage these as service container parameters you can reuse in other action service definitions but also in other dependencies without having to define them redundantly.
Now in case you do not want to expose all of that to the average user you could bundle some tools that generate the service container definitions from some more specialized syntax.
Agree with fabian : Approach 1-2 are the only one that allow conditional dependencies without creating new problems or concepts to learn (boilerplate code, ugly method signatures, WTF effects with magic variables names).
Approach 2 superseeds Approach 1, and I like that it is explicit and adheres to the Principle of Least Surprise, unlike 3-4. Just looking at the injection rules gives you a hint it's going to surprise some people.
If injecting the constructor solves the heavy-dependency problem with approach 2 I don't really see a clear necessity to complicate things as in Approach 4, only to have to document "best practices" later (that will often be ignored).
> On 3/22/10 4:12 PM, Jordi Boggiano wrote: >> On 22.03.2010 16:03, Fabien Potencier wrote: >>> It is in the new version of the RFC:
>> Ok my bad, sorry for acting like a victim ;)
>>> If there is a consensus about the Service suffix, I will have update the >>> RFC accordingly.
>> Great, I would favor $_Mailer because of laziness, but I will live with >> $mailerService, and I definitely agree it's more clear, although I'm >> sure many people will be confused with the notion of services at first, >> so either way it has to be well documented.
> I tend to prefer the Service suffix as this more explicit. The _ as a prefix has already a meaning in many PHP applications (protected variables).
22.03.2010, Χ 18:40, Bernhard Schussek ΞΑΠΙΣΑΜ(Α):
> Ad Approach 4:
> Prefixing or suffixing services still leaves one problem unattended: > What if DI variables (like $maxPerPage) collide with request > parameters? Naming it $maxPerPageService obviously makes no sense.
Maybe we can use "Service" suffix for services & "global" (or other) prefix/suffix for DI variables? I think, it will make sense:
public function showAction($blog, $mailerService, $globalTopicsPerPage) {}
Even if i prefer approach 1-2, using closures would be useful to declare optional services : public function showAction(Closure $mailer) { [...] if ($foo) { [...] $mailer()->send($mail); }
> On 22.03.2010, at 13:29, Fabien Potencier wrote:
>> On 3/22/10 1:27 PM, Lukas Kahwe Smith wrote:
>>> On 22.03.2010, at 13:18, Fabien Potencier wrote:
>>>> On 3/22/10 1:09 PM, Lukas Kahwe Smith wrote:
>>>>> Approach 1: >>>>> I do not like approach 1 because of the circular references and the fact that its not so easy to see the dependencies by just looking at the constructor (and action method) signatures.
>>>> Which circular reference? I don't see any.
>>> Isnt the controller also handled by the service container?
>> Nope. It is created by a listener.
> Ok, that surprised me :)
> In okapi2 we decided to have action classes be services as well. This way we get all the power of the service container import capabilities (remember you didnt want it in sfYaml .. now it seems you also added a trimmed down version to the routing config parsing), but also you can easily replace things (like say a bundle that you like depends on a bundle you dont like .. if they would be using services it would be easy to just replace the one you dont like with another implementation. Furthermore it makes handling of scalar/array dependencies easier, since then you can also manage these as service container parameters you can reuse in other action service definitions but also in other dependencies without having to define them redundantly.
> Now in case you do not want to expose all of that to the average user you could bundle some tools that generate the service container definitions from some more specialized syntax.
Controllers as services do not fit well with the Symfony DIC. I won't say it's not possible, but it feels really weird to me.
The DIC primary purpose is to manage "global" objects, objects that need to be referenced elsewhere (that's why it's named a container). It's very different from a factory. Using the DIC for controllers means that you will need to create a XML/YAML/PHP file to describe how to create EACH controller. That seems impractical and redundant to me. Why? Because the controller signatures already define the dependencies. So, in this case, I'd rather prefer to add DIC description via annotations. And even if we find a mechanism to automatically generate those descriptions, using the DIC is then moot, as you will loose the "flexibility".
You can also think about the controller loader class as yet another way to create a service via the DIC if that helps. In the implementation, everything will come from the DIC, so you will have all the flexibility of the DIC.
I have tons of other arguments against this idea. let's discuss these first if you want. As it's kind of difficult to discuss this by email, we can also meet on IRC; it will be more interactive and I think we will reach agreement faster ;)
