Document FQCN named controllers #7771

GuilhemN · Apr 8, 2017

This is possible since 2.7 but with 3.3 coming and the ability to have FQCN named services I think this is a more and more convenient notation that should be encouraged.

linaori · Apr 8, 2017

controller/service.rst

                class: AppBundle\Controller\HelloController

    .. code-block:: xml

        <!-- app/config/services.xml -->
        <services>
-            <service id="app.hello_controller" class="AppBundle\Controller\HelloController" />
+            <service id="AppBundle\Controller\HelloController" class="AppBundle\Controller\HelloController" />


Correct me if I'm wrong, but isn't this possible too?

<service id="AppBundle\Controller\HelloController" />

Yes it is, but only in 3.3. I think we should keep a notation that works on every supported version as it's not the main subject.

Your aim is at the master branch, hence I thought you were doing this for 3.3

Everything that used to work before 3.3 should probably target the respective lower branches in separate PRs.

@xabbuh you're right, I rebased this PR on 2.7.

@iltar I'll propose using the short notation in a new PR for 3.3, I should have done that since the beginning. Thanks for your comments!

linaori · Apr 8, 2017

controller/service.rst

        </services>

    .. code-block:: php

        // app/config/services.php
        use AppBundle\Controller\HelloController;

-        $container->register('app.hello_controller', HelloController::class);
+        $container->register(HelloController::class, HelloController::class);


Isn't something like this possible?

$container->register(HelloController::class);

I believe this also had a shortcut somehow.

Then we would have an example only working on 3.3, is it ok?

linaori · Apr 8, 2017

controller/service.rst

@@ -66,36 +66,39 @@ Then you can define it as a service as follows:

        # app/config/services.yml
        services:
-            app.hello_controller:
+            AppBundle\Controller\HelloController:
                class: AppBundle\Controller\HelloController


class can be omitted here as well

HeahDude · Apr 12, 2017

controller/service.rst

                class: AppBundle\Controller\HelloController

    .. code-block:: xml

        <!-- app/config/services.xml -->
        <services>
-            <service id="app.hello_controller" class="AppBundle\Controller\HelloController" />
+            <service id="AppBundle\Controller\HelloController" class="AppBundle\Controller\HelloController" />


I'm 👎 for changing service ids in 2.x docs, or we should do it absolutely everywhere.

It adds value only for controllers as it allows to use normal notations so I don't see the need to update all service ids. Also imo if it becomes the standard with 3.3 we should also promote it for 2.x.

What do you call "normal notations"?

With fqcn named services you can use all controller notations (e.g. AppBundle\Controller\HelloController::fooAction, App:Hello:foo, etc.). No need to define the service if you use @Route.

With other service ids you have to use the single colon notation: app.hello_controller:fooAction.

HeahDude · Apr 12, 2017

controller/service.rst


-    $this->forward('app.hello_controller:indexAction', array('name' => $name));
+    $this->forward('AppBundle:Hello:index', array('name' => $name));


Although I don't get why this is related to the fact that we use a FQCN as id.

Because this doesn't work with other service ids (fqcn named services are managed differently https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Controller/ContainerControllerResolver.php#L70).

Ok got it too :).

weaverryan · May 2, 2017

I think this is indeed a good idea! Can we update this to use annotations for routing (and of course also show yaml like we do in other places)? Before the class names, using @route was kind of a pain because you needed the extra annotation, but that's not true now.

Overall, I like this - good explanations of the different syntaxes!

javiereguiluz · May 3, 2017

@GuilhemN thanks for this contribution. Just asking: why #7864 proposes the same changes for the master branch? Isn't a duplicate of this one?

GuilhemN · May 3, 2017

@javiereguiluz it removes the class attributes which are useless on master (see 588333a)

javiereguiluz · May 3, 2017

I see. Then it makes sense. Thanks!

GuilhemN · May 3, 2017

@weaverryan I added an example with @Route, is this what you meant?

xabbuh · May 4, 2017

@GuilhemN Can you resolve the conflicts here?

…eguiluz) This PR was merged into the master branch. Discussion ---------- [3.3] Document FQCN named controllers Adaptation of #7771 for the master branch. Commits ------- 5fb2003 Minor reword 588333a [3.3] Document FQCN named controllers 49f589c Document FQCN named controllers

GuilhemN · May 4, 2017

Sure, it's done.

I can't rebase before tomorrow around 6pm but you can push on my repo if you want to merge this before.

GuilhemN · May 5, 2017

rebased

GuilhemN · May 10, 2017

Should I close this ?
We can apply the change of #7864 to older branches to accomodate people faster to the use of fqcn controllers but that's not mandatory. Having this only in 3.3 is good enough for me so that's up to you :)

robfrawley · May 11, 2017

I think we should keep this in 3.3 only. We shouldn't go changing the older docs out from under people. It will make them not align with other tutorials and external documentation for the 2.x branch, IMHO.

GuilhemN · May 11, 2017

It will make them not align with other tutorials and external documentation for the 2.x branch, IMHO.

I don't think that's a problem, we often change our practices even in older branches.

Anyway, I'm closing this, I think the gain is not so big in 2.x because you have to duplicate the class attribute.

carsonbot added the Status: Needs Review label Apr 8, 2017

linaori reviewed Apr 8, 2017

View reviewed changes

HeahDude added Status: Needs Work and removed Status: Needs Review labels Apr 11, 2017

GuilhemN changed the base branch from master to 2.7 April 12, 2017 17:21

GuilhemN force-pushed the CONTROLLERASASERVICE branch from e5285f3 to 99ba301 Compare April 12, 2017 17:23

carsonbot added Status: Needs Review and removed Status: Needs Work labels Apr 12, 2017

HeahDude reviewed Apr 12, 2017

View reviewed changes

GuilhemN mentioned this pull request May 1, 2017

[3.3] Document FQCN named controllers #7864

Merged

javiereguiluz approved these changes May 3, 2017

View reviewed changes

javiereguiluz added Status: Reviewed and removed Status: Needs Review labels May 3, 2017

GuilhemN and others added 3 commits May 5, 2017 17:40

Document FQCN named controllers

3d25905

Minor reword

14f89d7

Add an example with @route

280a5c7

GuilhemN force-pushed the CONTROLLERASASERVICE branch from 7b53ef2 to 280a5c7 Compare May 5, 2017 15:40

GuilhemN closed this May 11, 2017

GuilhemN deleted the CONTROLLERASASERVICE branch May 11, 2017 16:02


		$this->forward('app.hello_controller:indexAction', array('name' => $name));
		$this->forward('AppBundle:Hello:index', array('name' => $name));

Search code, repositories, users, issues, pull requests...

Uh oh!

Document FQCN named controllers #7771

Document FQCN named controllers #7771

Uh oh!

Conversation

GuilhemN commented Apr 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuilhemN Apr 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeahDude Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuilhemN Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan commented May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javiereguiluz commented May 3, 2017

Uh oh!

GuilhemN commented May 3, 2017

Uh oh!

javiereguiluz commented May 3, 2017

Uh oh!

GuilhemN commented May 3, 2017

Uh oh!

xabbuh commented May 4, 2017

Uh oh!

GuilhemN commented May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GuilhemN commented May 5, 2017

Uh oh!

GuilhemN commented May 10, 2017

Uh oh!

robfrawley commented May 11, 2017

Uh oh!

GuilhemN commented May 11, 2017

Uh oh!

Uh oh!

GuilhemN commented Apr 8, 2017 •

edited

Loading

GuilhemN Apr 8, 2017 •

edited

Loading

HeahDude Apr 12, 2017 •

edited

Loading

GuilhemN Apr 12, 2017 •

edited

Loading

weaverryan commented May 2, 2017 •

edited

Loading

GuilhemN commented May 4, 2017 •

edited

Loading