[DX] ParamConverter options simplification

[weaverryan]

I'd like to propose a simplification of some of the options for ParamConverter (as well as making sure the documentation is rock-solid for how to use the different options).

Currently, even without the annotation, if you type-hint an argument, ParamConverter will query Doctrine for the entity automatically (I believe using all of the wildcard values in the where statement). That's cook. But if you need to do something custom, it gets ugly.

Currently

/**
 * @Route("/blog/{date}/{slug}/comments/{comment_slug}")
 * @ParamConverter("post", options={"mapping": {"date": "date", "slug": "slug"}})
 * @ParamConverter("comment", options={"mapping": {"comment_slug": "slug"}})
 */
public function showAction(Post $post, Comment $comment)
{
}

What about:

1. Allowing "mapping" to be used outside of options
2. Adding new "key" and "keys" configuration you can use if the key in the route matches your property name (or perhaps mapping could be made to do this automatically, if it detects the indexes are integers instead of strings?)

/**
 * @Route("/blog/{date}/{slug}/comments/{comment_slug}")
 * @ParamConverter("post", keys={"slug","date"})
 * @ParamConverter("comment", mapping={"comment_slug": "slug"})
 */
public function showAction(Post $post, Comment $comment)
{
}

Or could we leverage the expression language? I've experimented with some syntaxes, but nothing has felt better than just writing PHP code.

[javiereguiluz]

@weaverryan I really like the idea of improving the syntax of the param converters. I've come up with the following (crazy) proposal. What do you think?

// The common use-case
// ----------------------------------------------------------------------------

use Sensio\BlogBundle\Entity\Post;

/**
 * @Route("/blog/{id}")
 * @ParamConverter("post", class="SensioBlogBundle:Post")
 */
public function showAction(Post $post) { }

/**
 * @Route("/blog/{id}")
 * @Query("post")
 */
public function showAction(Post $post) { }


// Using a custom repository method to perform the query
// ----------------------------------------------------------------------------

use Sensio\BlogBundle\Entity\Post;

/**
 * @Route("/blog/{id}")
 * @ParamConverter("post", class="SensioBlogBundle:Post", options={"repository_method" = "findWithJoins"})
 */
public function showAction(Post $post) { }

/**
 * @Route("/blog/{id}")
 * @Query("post", method = "findWithJoins")
 */
public function showAction(Post $post) { }


// Using a custom mapping of route params to entity fields
// ----------------------------------------------------------------------------

use Sensio\BlogBundle\Entity\Post;
use Sensio\BlogBundle\Entity\Comment;

/**
 * @Route("/blog/{date}/{slug}/comments/{comment_slug}")
 * @ParamConverter("post", options={"mapping": {"date": "date", "slug": "slug"}})
 * @ParamConverter("comment", options={"mapping": {"comment_slug": "slug"}})
 */
public function showAction(Post $post, Comment $comment) { }

/**
 * @Route("/blog/{date}/{slug}/comments/{comment_slug}")
 * @Query("post", with = {"date", "slug"})
 * @Query("comment", with = {"comment_slug": "slug"})
 */
public function showAction(Post $post, Comment $comment) { }


// Excluding route params from the query
// ----------------------------------------------------------------------------

use Sensio\BlogBundle\Entity\Post;

/**
 * @Route("/blog/{date}/{slug}")
 * @ParamConverter("post", options={"exclude": {"date"}})
 */
public function showAction(Post $post, \DateTime $date) { }

/**
 * @Route("/blog/{date}/{slug}")
 * @Query("post", without = {"date"})
 */
public function showAction(Post $post, \DateTime $date) { }


// Using a custom route param as primary key for the query
// ----------------------------------------------------------------------------

use Sensio\BlogBundle\Entity\Post;
use Sensio\BlogBundle\Entity\Comment;

/**
 * @Route("/blog/{id}/comments/{comment_id}")
 * @ParamConverter("comment", class="SensioBlogBundle:Comment", options={"id" = "comment_id"})
 */
public function showAction(Post $post, Comment $comment) { }

/**
 * @Route("/blog/{id}/comments/{comment_id}")
 * @Query("comment", with = {"id" : "comment_id"})
 */
public function showAction(Post $post, Comment $comment) { }

@fabpot there are a lot of developers that would like a simpler and more expressive syntax for ParamConverters. Is this something that you support? In case you do, is this issue going in the right direction or do you have alternative ideas? Thanks!

[fabpot]

I do support the idea.

But renaming to Query does not make to me as a param converter is about converting request parameters to objets, so Query is far from what it actually does.

[javiereguiluz]

It's great to read that. Let's see if the community can propose new syntaxes to simplify this.

I knew that you wouldn't like @Query :) Anyway, this was my rationale:

When someone asks me what does the following mean:

@ParamConverter("post", options={"exclude": {"date"}})

I always tell him/her: it just queries the database using Doctrine to get a Post object and the query is done without using the "data" route param.

In my opinion, the key is "query database" + "post" + "without date". And that's why I proposed:

@Query("post", without = {"date"})

The Query proposal would be only for Doctrine/Propel ParamConverter, so the @ParamConverter annotation would remain for other cases.

[mmoreram]

I would be nice to support the option of creating an entity using a factory.

I am used to using ParamConverters in my projects, I really like them, even knowing that are just Annotations (another RFC). But I am also used to working with factories, so the way actual ParamConverter just instances all entities is with a simple new.

I solved that using a multi-format.

https://github.com/mmoreram/ControllerExtraBundle#entity-provider

I also created a new @Entity annotation just with simple (and useful) features. Easy to understand, and already documented.

https://github.com/mmoreram/ControllerExtraBundle#entity

@weaverryan Do you think we could reuse any concept? Feel free to make me know, I would love to conttribute with this redesign :)

[tvlooy]

Apart from this, I don't like the way the repository is used. See this related PR #271 I will pick this up and finish that implementation as soon as possible.

[juliensnz]

What bother me the most about param converters is that it's only an annotation. A configuration file could be so much nicer (to override it more easily).

I would also like to see a mode flexible class parameter. Right now it supports only the FQCN or the AcmeDemoBundle:Entity form. It could be nice to use a container parameter to refer the class.

/**
* @ParamConverter("comment", class="%pim_enrich.entities.product%")
*/

I did a small POC for the Akeneo PIM project : https://github.com/akeneo/pim-community-dev/blob/bc02a2a2e4fcea5cffe84f9d9b9795805f18237c/src/Pim/Bundle/EnrichBundle/ParamConverter/ConfigurableParamConverter.php

[weaverryan]

Hi guys!

I think first, we should solve a simple problem: allowing request attributes (i.e. routing wildcards) to be transformed (usually, Doctrine query) into an object. @mmoreram you have some nice features - but maybe not all of that needs to be in core :).

Thinking about this one small problem - and how to solve it in the simplest way possible, I thought of this:

/**
 * @Route("/post/{postId}")
 * @DoctrineConverter("post", "repository.find(postId)")
 */
public function showAction(Post $post)

I don't want to replace things that can be done easily in PHP with more obtuse configuration.

[mmoreram]

@weaverryan Yes, sure, I was wondering only to show some ideas :) I think that factories and other formats would be the less magical way (similar way than DI can instance a service using factorying, or defining the class with a parameter, like @sumbobyboys says)

Once again, very interested in the RFC and the development :)

[hhamon]

@weaverryan I really like your last proposal with  @DoctrineConverter. It makes much more sense written like this.

[weaverryan]

Fwiw, I showed a few sflive attendees the DoctrineConverter syntax and they found it simple to understand.

[javiereguiluz]

What I dislike is hardcoding the Doctrine name and adding the long/boring Converter suffix. Couldn't we use some generic word to describe what the converter is doing? @Fetch, @Find, @Get, @Query, ...

[hhamon]

@Entity or @Record instead?

[javiereguiluz]

I think it should be a verb because the ParamConverter is an action:

/**
 * @Fetch( ... )
 * @Find( ... )
 * @Get( ... )
 * @Query( ... )
 * @Convert( ... )
 * @Transform( ... )
 * ...
 */

[hhamon]

It's an action but it represents the object to be injected.

[sandermarechal]

Just throwing in a wild idea here. I recently build a largish Symfony application that uses ParamConverter a lot. The most annoying thing for me isn't that the options can get complicated, but that I need to repeat the same annotation over-and-over on different controller actions. And when something changes I need to go back and update all those annotations.

Perhaps we should separate the "what" and the "how"? Perhaps move the bulk of the configuration to a separate config file or mapping, or to a service?

Maybe we can even do away with a per-method annotation all together. Why not have the option to use a class annotation that tells you to use a parameter converter for all methods in a class. Why not have application-wide parameter converters that apply to all controllers/methods in an application or bundle?

[frastel]

ParamConverters currently try to "magically" convert some request parameters to some other value, independent to the actual route and targeted action and that's in my opinion the most problematic point to use those converteres in a way @sandermarechal requests it.

How about the approach of generating a route based ConverterRegistry which could be filled with tagged services?

#routing.yml
blog_show:
    path:      /blog/{slug}
    defaults:  { _controller: AcmeBlogBundle:Blog:show }

#services.yml
services:
    foo.converter.blog:
        class: Acme\HelloBundle\Request\ParamConverter\BlogParamConverter
        tags:
            -  { name: application.param_converter, route: blog_show }
            -  { name: application.param_converter, route: some_other_route }

The ConverterRegistry is called within some RequestListener and when the actual route matches the Converter is executed. In this way you could write your Converters specific to your routes and actions. Besides they could be re-used all over the application. It's completely up to the developer then how she manages and implements those Converters and no additional configuration at all is needed then. It's like @weaverryan already said: This case could be done best within plain PHP and configuration is just overhead.

Con of this approach: Route names get more duplicated and spread over other configuration files.

[juliensnz]

@frastel :+1:

[sandermarechal]

@frastel To prevent duplication, perhaps instead of tagging services with routenames, you can add the required converters to the route configuration? Alternatively, make tagging optional and select converters by parameter class first. Chances are that if you have multiple routes/actions with a "Post" parameter, the same converter is needed for them all. So just use route tags for the exception.

[weaverryan]

Implemented in #432