Some questions/feedback about docs/usages

[linaori]

I have to admit, I've not used the package yet. Implementing this package in 1 applications is not done, would have to streamline it between apps, so here's some feedback as asked :smile:

(1) I don't use autowiring so I will need to explicitly define the service definitions. I have a lot of Controllers already registered but not under the FQCN, would this register controllers twice?

(2) Why do I have to define the class AND the ID with the same value?

services:
    'AppBundle\Action\MyAction':
        # the class value looks obsolete, ID should already be sufficient right?
        class: 'AppBundle\Action\MyAction'
        arguments: [ '@router', '@twig' ]

(3) At the moment I define the service definitions via an annotation route loader and the SensioFrameworkExtraBundle: @Route(service="...").

This bundle also hooks into the Routing Component (if it is available): when the @Route annotation is used as in the example, the route is automatically registered: the bundle guesses the service to map with the path specified in the annotation.

Would this mean I can remove the class level service option for the @Route annotations?

I have a guess on what the "tag" configuration does, but it's not really well documented. Does the key mean it checks if the given class is a subclass of?

[dunglas]

Hi,

Thanks for this feedback!

(1) Indeed, you'll have to: not use this bundle (its responsibility is only to register automatically the controller as a service and enable the autowiring for it) or to register controllers twice. (2) It is basically to make (3) working :) It was not the case initially, but the name of the service has been changed to match what is done for forms internally in Symfony. See https://github.com/dunglas/DunglasActionBundle/issues/30 (3) Yes! But this bundle doesn't do anything specific for that (it's directly in Symfony core), the trick is to enable is to have the FQN as service name.

Regarding tags, yes, it's exactly how it works: https://github.com/dunglas/DunglasActionBundle/blob/master/DependencyInjection/DunglasActionExtension.php#L138-L144

[linaori]

@dunglas regarding (2), isn't it possible to simply add the "class" based the service id if the class exists and is not defined in the configuration? Because this double writing down is one of the issues that's keeping me from it. I can't benefit from the automatic registering with autowiring thus it's actually double work for me to write down the classes.

This should be possible afaik :smile:

services:
    'AppBundle\Action\MyAction':
        arguments: [ '@router', '@twig' ]
        tags: ~

I'm all for write less, do more, as long as it doesn't hit performance/DX and remains as close to stock as possible.

[GuilhemN]

For (2), an integration with https://github.com/symfony/symfony/pull/18300 would be awesome imo (to not register already registered services). I have to find some time to reopen it...

[linaori]

@Ener-Getick I'm good enough with actually storing the id = class. What about a compiler pass that iterates over all definitions that do not have a class and fill the class with the service id if class_exists($serviceId);?

[GuilhemN]

@iltar the benefit would be minimal here as this bundle is useless if you don't use autowiring (it is allowing to avoid manually defining some of your services). But why not in another context, it may be considered a good practice to name its services according to their FQCN. However this has to be proposed on the sf repo.

[Wirone]

@dunglas I would like to ask question, so I will do it here since it's still open:

(4) What about ParamConverters? In standard controller-based Symfony convention there's possibility to typehint action's argument with class supported by registered request listener, which will convert raw value to typehinted object, so it can be used directly. What about your __invoke(), is only Request passed there everytime and logic from param converter must be moved to action's class?

[dunglas]

@Wirone param converters should work. There is nothing specific to __invoke in this bundle, it's just a standard action method and it has builtin support in Symfony.

[Wirone]

Ok, thanks @dunglas. I did not use it yet, it was my first doubt after reading your post. I will give it a try in the near future :-)

[dunglas]

Closing as most concerns have now been fixed or answered.