-
Notifications
You must be signed in to change notification settings - Fork 180
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for component argument resolution #214
base: master
Are you sure you want to change the base?
Add support for component argument resolution #214
Conversation
0150f44
to
7acbf62
Compare
7acbf62
to
56f8620
Compare
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.
Hello @hugochinchilla
Thanks to propose the auto-wiring feature for the custom DIC implemented on this version of Symfony.
However, in my honest opinion it is useless to work on this part as Symfony2+ have already an implementation.
The best thing to do will be replace it with the Symfony2+ DIC. This way we will not maintain anymore this version of DIC. It is not so hard to do it.
About the code you wrote:
- There are not test cases about this new code.
👎
@alquerci I'm not sure what are you proposing about using symfony2+ DIC, I worked with what is available on this version, I use my own container but took the effort of making this compatible with the default DIC implementation but easy enougth to swap implementations with custom ones. If you are sugesting that this project should adpot symfony2+ DIC I agree with you but this is outside the scope of this proposal, it would be easy to change it all on a future iteration. I have provided unit tests to check the correct behavior of the parameter resolver and also some functional tests to make sure all the integration works as expected. |
@hugochinchilla The concern is that implementing auto-wiring is more harder than just doing what you done. IMHO the only issue here is a conceptual bug about the non ability to extends the controller call method. Adding this ability is fully on the migration path to Symfony2+. So I suggest you to provide a way to customize the controller method call to add the ability to everyone to use there own choice. For example on Symfony2 (which is compatible with PHP 5.3 like as symfony1) the resolution is based on request parameter. The harder will be on the controller helper but it is still possible to do it. |
@alquerci again I'm not sure what you are asking here. When you say I should add a way to customize the controller call method what are you asking for? If you mean a way to customize which method the controller should execute this is already supported. If you mean another thing please explain with more detail (some pseudocode will help). Also the purpose of this PR is not to add autowiring, autowiring can be acomplished by using another DIC which supports it and I already provided example code on how to use another container to get even more funcionality from this PR. |
An example of code on public function execute($request)
{
// dispatch action
$actionToRun = 'execute'.ucfirst($this->getActionName());
// ....
return $this->doCallController($request, $actionToRun);
}
protected function doCallController($request, $actionToRun)
{
return $this->$actionToRun($request);
} This way it just need to overwrite This way there is no need to add additional code as it fix the main issue. |
This adds support to resolve arguments using the DIC.
Example usage:
Caveats
It works well for classes extending
sfActions
andsfComponents
, it can be used with classes extendingsfAction
orsfComponent
but the method signature must be compatible with the one in the parent so you will need to add the default valuenull
to avoid an error.Using a different container
I's been implemented in a way that is extremly easy to use another DIC to provide the requested arguments, we use this with a Psr compilant DIC and the code looks like this:
in
services.yml
: