Adding an experimental PreExecuteListener

[weaverryan]

Hi guys!

This is an idea that I wanted feedback on. With this, if you controller implements a new interface, then a preExecute() method will be called before executing the action.

The problems I'm hoping to make smoother:

1. Applying security to most or an entire controller
2. More powerful param converters, because they can be done in PHP - the ParamConverter configuration is ugly, and not everyone likes annotations anyways.
3. Because of (2), the possibility that @Security can be used more easily, when you're checking against objects:

// use statements ...

class CategoryController extends Controller implements PreExecuteInterface
{
    public function preExecute(Request $request, $methodName)
    {
        $this->denyAccessUnlessGranted('ROLE_ADMIN');

        if ($id = $request->attributes->get('id')) {
            $categoryRepository = $this->getDoctrine()
                ->getRepository('AppBundle:Category');

            $category = $categoryRepository->find($id);
            if (!$category) {
                throw $this->createNotFoundException();
            }

            $request->attributes->set('category', $category);
        }
    }

    /**
     * @Route("/category/{id}", name="category_show")
     * @Security("is_granted('CATEGORY_SHOW', category)")
     */
    public function showCategoryAction($category)
    {
        return $this->render('fortune/showCategory.html.twig',[
            'category' => $category
        ]);
    }
}

What do you think of this? Would you use it or not? Any ideas/improvements?

Thanks!

[hhamon]

Hi Ryan,

Great addition! What about the order of execution? Is the preExecute() method executed prior to the kernel.controller event listeners or after them just before the controller is invoked. I guess order may matter.

[weaverryan]

Hey Hugo!

Right now, the listener that calls preExecute() is a listener on kernel.controller, and I've given it a priority of 10. That's because I think it's better before normal listeners - especially @Security, which can use any request attributes we add. This may not fit every scenario, but I think it's best.

Thanks!

[webda2l]

Nice! Can be useful

[timglabisch]

👍

[fabpot]

I don't see the need for an interface here. I like being explicit in controllers, and adding an explicit call to ->preExecute() does not seem like a big cognitive overhead. And we know that people will then like a postExecute() method, and they will want to get access to the arguments passed to the action method, and they will want to be able to apply annotations on it as well.

It looks like it does not really solve a problem that can already be solved quite naturally and without any magic.

[xabbuh]

I think you should do a check first if $controllerObject really is an object.

[linaori]

I agree with @fabpot. If you really want to execute that same piece of code for every controller there are alternative solutions of which the most logical: Move it to a service. If you would extract the method of the controller, the page behavior will change because the new Controller most likely won't have that filter.

[weaverryan]

Closing. My biggest goal was to help making "param converting" easier (for example, the syntax for param converting Doctrine entities is ugly). But I'm going to tackle that in a different way - #321.

Thanks!