Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[HttpKernel] Before and after methods in controllers (Kohana Style) #1975

Closed
ricardclau opened this Issue Aug 17, 2011 · 26 comments

Comments

Projects
None yet
Contributor

ricardclau commented Aug 17, 2011

It would be a nice addition to Symfony2 framework to be able to define before and after methods in controllers.

This is one of the features I like best in Kohana3 and I think it is extremely useful.
If you think this could be a good feature I can patch HttpKernel class and send a pull request for this.

Any comments on that will be much appreciated.

Contributor

xaav commented Aug 17, 2011

Guess what: PHP already has these functions built in! They're called __construct and __destruct. Isn't it great to use PHP's existing functions, and not create custom redundant methods to clutter your classes?

Member

stof commented Aug 17, 2011

The HttpKernel component currently allows to use any callable as controller (in Symfony2, it is (quite) always classes because of the way the framework uses it but in Silex it is often a closure for instance). So adding this would make the component far less flexible.
To add logic before the action (but after the controller is retrieved), you can use the kernel.controller event. You could for instance register a listener checking that the controller is an object implementing a given interface and then calling the before() action on it, or a listener that contains some shared logic...
And to put logic after the controller, there is the kernel.response event (and kernel.view when you don't return a Response directly).

Contributor

lsmith77 commented Aug 17, 2011

yes .. events are the way to go here. if you want controller/action specific before/after logic then imho its better to call it manually though you could build something like this using events as well.

Contributor

xaav commented Aug 17, 2011

I think it's clear that this is for legacy frameworks that don't use the new features of PHP5, and that this issue should be closed.

Contributor

lsmith77 commented Aug 17, 2011

well i have used features like this as well in the past, so i can totally see where this request is coming from. it takes a while to warp the mind from the past into Symfony2 style :)

Member

stof commented Aug 17, 2011

@xaav No, these PHP5 features don't correspond to what is requested here.
__destruct may be called far after the end of the action as it depends of the time where it is garbage collected (for a controller defined as service, it is only when the container itself is garbage collected at the very end of the request).
And __construct is not the good place as this occurs before the controller is ready to be used: part of the initialization is done through setters, be it when using a service (setter injection may be used) or when using a container-aware controller (the container is injected through a setter)

Contributor

ricardclau commented Aug 17, 2011

Sorry guys, I was on my way home and did not expect such fast amount of replies.

@stof has absolutely understood my point. I need some logic to be executed after the controller is prepared. In my case I need the container object, so __construct and __destruct are not the places for such features. However, I was thinking 100% controller oriented, didn't think about callable objects and I agree that flexibility would be lost.

Yes, I know about event listeners and have developed one for my purpose but unfortunately didn't work for me. I want the logic to be executed in some controllers but not at every request. I tried kernel.request, kernel.controller and kernel.view but the are all executed at every request. Maybe I am missing something.

Is it possible to define listeners for some controllers without using regexp over querystring?

Apart from that I want to check some things and if request is not as I expect, I want to throw an Exception to be rendered in a {_format} way (I mean either html or json, depending on that param), so in Listeners (as far as I know) my exception would render always a text/html response. Again, maybe I am missing something as I am quite new to Symfony2.

Of course, I can call a method at the beginning of every action but I find these a little bit against the DRY principle.

To expose my case in a "real" example, I am developping an API which can be accessed in HTML / JSON and in the future XML format. Some controllers respond always, some need a token to be checked and some need further logic before executing the action requested. Actually it is similar to a ACL authentication, maybe I should check how securityBundle and others for that purpose deal with this problem.

Again, any comments / orientation is 100% welcome!

@xaav I don't understand your contemptous tone. I think I asked respectfully and if at least Kohana and Zend FW (with the _init* methods) have these features, maybe this is something useful. Beyond that, let me say that I have been working with PHP5 for several years and I am Zend Certified so I know perfectly what __construct and __destruct do, specially __destruct that gets executed in garbage collection or unset so you have no real control on when it gets executed.

Owner

fabpot commented Aug 17, 2011

@ricardclau: symfony1 has what you are looking for (preExecute() and postExecute()). But Symfony2 is a bit different. As you have noted, you can do that by listening to some events (like kernel.controller and kernel.response for instance). If you want to restrict to some controllers/actions, you have access to the Request, to that's totally possible. You can also use the RequestMatcher class to help you define the restrictions. Another possibility is to create a base class for the controllers that need this special logic.

Member

stof commented Aug 17, 2011

@ricarclau The kernel.controller event gives you access to the controller matched by the request. $event->getController() returns the callable (so generally something like array($controllerObject, 'myAction') in the framework). So you can do some logic based on the controller you get (checking that it implements a given interface for instance). Here is an implementation that achieve what you ask for:


<?php
namespace Acme\DemoBundle\EventListener;

use Symfony\Component\HttpKernel\Event\FilterControllerEvent;

class BeforeControllerListener
{
     public function onKernelController(FilterControllerEvent $event)
     {
           $controller = $event->getController();
           if (!is_array($controller)) {
                // not a object but a different kind of callable. Do nothing
                return;
            }

            $controllerObject = $controller[0];
            if ($controllerObject instanceof InitializableControllerInterface) {
                   $controllerObject->initialize($event->getRequest());
                   // this method is the one that is part of the interface.
            }
     }
}

Regarding the exception handling, the format should be taken into account if it occurs after the RouterListener is called (as the format is not set from the route before that). So for most listeners, it will be used.

Contributor

schmittjoh commented Aug 17, 2011

You can do even better by using a listener like this (then you'll effectively have the pre/post execute events that symfony1 had) and actually the JMSSecurityExtraBundle uses this for ACL checks:

<?php

class Listener
{
    public function onKernelController($event)
    {
        $currentController = $event->getController();
        $newController = function() use ($currentController) {
            // pre-execute
            $rs = call_user_func_array($currentController, func_get_args());
            // post-execute

            return $rs;
        };
        $event->setController($newController);
    }
}

The only caveat here, make sure to set a very low priority (something below -255).

Member

stof commented Aug 17, 2011

@schmittjoh but this means that the pre-execute and post-execute logic is written in the listener, not in the controller.

Contributor

schmittjoh commented Aug 17, 2011

Why? It can be wherever you want (controller, another service, listener,
etc.)

On Wed, Aug 17, 2011 at 9:47 PM, stof <
reply@reply.github.com>wrote:

@schmittjoh but this means that the pre-execute and post-execute logic is
written in the listener, not in the controller.

Reply to this email directly or view it on GitHub:
#1975 (comment)

Member

stof commented Aug 17, 2011

well, the listener add the same pre-execute code to all controllers in your case, unless you add more complicated logic achieving what I do in my example (in which case I'm not sure your solution is better).

Contributor

ricardclau commented Aug 17, 2011

Again, I am amazed by the fast and top-level answers. Thank you all for your time and solutions/approaches proposed.

I won't be able to dive into it until tomorrow but I think I will find a clean way to go based on all that @fabpot, @stof and @schmittjoh have said.

Of course, the issue can be closed but I would like to contribute to this discussion thread with my final solution, as it may be useful for some other developers as all the code you have exposed.

I will also post my solution in my personal blog as I guess I am not the only Kohaner in his way to adopt Symfony2 ;). I have seen some people asking for the same in some websites so it may be useful.

Thanks again and regards

@stof stof closed this Aug 17, 2011

Contributor

Gregwar commented Aug 18, 2011

I'm just a spectator of this issue, and I think that this could be a good idea for the cookbook, especially the @stof implementation which is generic.

Even if the Symfony2 philosophy is quite different, people coming from other frameworks are used putting before and after filters and they may like having an article about how to do that "in a clean way".

Member

stof commented Aug 18, 2011

@Gregwar open a ticket on the doc repo :)

Contributor

ricardclau commented Aug 18, 2011

Hi again!

Just to mention that both methods work just fine and I am still deciding which way to go.

@schmittjoh method is exactly as kohana or old symfony1 before/after filters and @stof instanceof interface approach seems to be more customizable.

Regarding what I said about the exception rendering, the problem was that in my first implementation, my listener acted on every controller request and executed twice, one for the actual Controller and a second time for the ExceptionController. Then, the "oops, something went wrong html page was rendered" as both controller events threw an Exception. So, it actually worked fine, I only had to check that "it was not an exceptionController" to apply my filter.

So, thanks again for your help and I absolutely agree that this fits perfectly in the cookbook as most people is used to these kind of filters and at first sight we miss them!

Just cross referencing with Gregwar's doc ticket:
symfony/symfony-docs#649

Contributor

ricardclau commented Aug 21, 2011

Just for the record, I made a post about this whole story in my blogsite http://www.ricardclau.com/2011/08/kohana-style-before-and-after-methods-in-symfony2/. Although most of my posts are written in Spanish, this one goes in English.

You can get any ideas you want to write the example in the cookbook!

Member

stof commented Aug 21, 2011

Just a though about your implementation: you are whitelisting the exception controller, but this means that the code will be executed for every other controller, including the webprofiler for instance, which could be an issue.

Also, note that you should inject the dependency of your listener (namely the tokens parameter) instead of the whole container. This would be a clean dependency injection pattern.

Contributor

ricardclau commented Aug 21, 2011

You're absolutely right about the first issue, is there any interface that means "all my controllers but not exception, webprofiler, etc..."?

And regarding your second statement...how do you inject just a part of the configuration container and not the whole thing? I have no idea on how to do this with the YML file. Have you got any example for that?

Thanks for having a look to my post and correcting my poor implementations :)!

Member

stof commented Aug 21, 2011

For the interface, this is part of what I suggested: create an interface (possibly empty) and implement it in your controllers (it would even allow having some public controllers, solving your TODO by simply separating secured actions from public ones in different controllers)

For the service, the YAML syntax is

my_listener:
      class: ....
      arguments:
            - "%tokens%"

The doc about it is here

Contributor

ricardclau commented Aug 21, 2011

Ok, sounds much better to have empty interfaces to separate controllers. I though to implement your solution in a further development stage (right now we don't need 100% public actions) but the webprofiler thing has absolutely convinced me. With the interface approach there will be nothing uncontrolled.

Thanks for the YAML information, I am ashamed on how easy it is, I still need to study the existing documentation furtherly.

Anyway, it is amazing how everything has been thought and re-thought in Symfony2. As I say in my blog entry, Symfony2 just needs a bigger cookbook to get everyone convinced.

Right now I have no time to correct and test the code in the entry, but tomorrow I will rewrite it as you suggested and get this fixed.

Thanks for your time and comments and best regards

@schmittjoh with your solution, in case when we are using annotations for routes func_get_args() returns nothing, which of course means nothing is passed to controller action. I'm not sure if this is an issue or I'm doing something wrong.

egoista commented May 15, 2015

Sorry for disagree, especially in a closed issue, but IMHO have cases when you need to execute something before the actions of just one controller.

For example I have a entity address, and many others entity have address, when I need to create/edit/delete address for any other entity I use the same controller and after I redirect it back to the referer, so if the call to address controller dont have a referer I send to index.

Do a listener for this and check for just one specific controller is to much (and actually IMHO wrong).

Maybe Symfony can have something like doctrine haslifecyclecallbacks http://doctrine-orm.readthedocs.org/en/latest/reference/annotations-reference.html#annref-haslifecyclecallbacks

Member

jakzal commented May 15, 2015

@egoista I don't see why you would need to do it outside of a controller. A method call in each action method is far more readable than trying to introduce indirection with events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment