symfony/symfony-docsPublic

NotificationsYou must be signed in to change notification settings
Fork5.3k
Star2.3k

Document FQCN named controllers#7771

New issue

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

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

GuilhemN wants to merge3 commits intosymfony:2.7fromGuilhemN:CONTROLLERASASERVICE

Closed

Document FQCN named controllers#7771

GuilhemN wants to merge3 commits intosymfony:2.7fromGuilhemN:CONTROLLERASASERVICE

Conversation

Copy link

Contributor

GuilhemN commentedApr 8, 2017•
edited
Loading

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.

carsonbot added the Status: Needs Review label

Apr 8, 2017

linaori reviewed

Apr 8, 2017

View reviewed changes

controller/service.rst

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

<serviceid="AppBundle\Controller\HelloController" />

Copy link

ContributorAuthor

GuilhemNApr 8, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@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 reviewed

Apr 8, 2017

View reviewed changes

controller/service.rst

		use AppBundle\Controller\HelloController;

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Isn't something like this possible?

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

I believe this also had a shortcut somehow.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

controller/service.rst

		services:
		app.hello_controller:
		AppBundle\Controller\HelloController:
		class:AppBundle\Controller\HelloController

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

class can be omitted here as well

HeahDude added Status: Needs Work and removed Status: Needs Review labels

Apr 11, 2017

GuilhemN changed the base branch frommaster to2.7

April 12, 2017 17:21

GuilhemN force-pushed theCONTROLLERASASERVICE branch frome5285f3 to99ba301Compare

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

controller/service.rst

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

Copy link

Contributor

HeahDudeApr 12, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Contributor

HeahDudeApr 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What do you call "normal notations"?

Copy link

Contributor

HeahDudeApr 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ok got it.

Copy link

ContributorAuthor

GuilhemNApr 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 reviewed

Apr 12, 2017

View reviewed changes

controller/service.rst

		defined above with the id ``AppBundle\Controller\HelloController``::

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

Copy link

Contributor

HeahDudeApr 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

GuilhemNApr 12, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

HeahDudeApr 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ok got it too :).

GuilhemN mentioned this pull request

May 1, 2017

[3.3] Document FQCN named controllers#7864

Merged

Copy link

Member

weaverryan commentedMay 2, 2017•
edited
Loading

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 approved these changes

May 3, 2017

View reviewed changes

Copy link

Member

javiereguiluz commentedMay 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?

javiereguiluz added Status: Reviewed and removed Status: Needs Review labels

May 3, 2017

Copy link

ContributorAuthor

GuilhemN commentedMay 3, 2017

@javiereguiluz it removes theclass attributes which are useless on master (see588333a)

Copy link

Member

javiereguiluz commentedMay 3, 2017

I see. Then it makes sense. Thanks!

Copy link

ContributorAuthor

GuilhemN commentedMay 3, 2017

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

Copy link

Member

xabbuh commentedMay 4, 2017

@GuilhemN Can you resolve the conflicts here?

xabbuh added a commit that referenced this pull request

May 4, 2017

feature#7864[3.3] Document FQCN named controllers (GuilhemN, javier…

a9f975b

…eguiluz)This PR was merged into the master branch.Discussion----------[3.3] Document FQCN named controllersAdaptation of#7771 for the master branch.Commits-------5fb2003 Minor reword588333a [3.3] Document FQCN named controllers49f589c Document FQCN named controllers

Copy link

ContributorAuthor

GuilhemN commentedMay 4, 2017•
edited
Loading

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.

GuilhemNand others added3 commits

May 5, 2017 17:40

Document FQCN named controllers

3d25905

Minor reword

14f89d7

Add an example with@route

280a5c7

GuilhemN force-pushed theCONTROLLERASASERVICE branch from7b53ef2 to280a5c7Compare

May 5, 2017 15:40

Copy link

ContributorAuthor

GuilhemN commentedMay 5, 2017

rebased

Copy link

ContributorAuthor

GuilhemN commentedMay 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 :)

Copy link

Contributor

robfrawley commentedMay 11, 2017

I think we should keep this in3.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 the2.x branch, IMHO.

Copy link

ContributorAuthor

GuilhemN commentedMay 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 theclass attribute.

GuilhemN closed this

May 11, 2017

GuilhemN deleted the CONTROLLERASASERVICE branch

May 11, 2017 16:02

Labels

Status: Reviewed

Movatterモバイル変換

Uh oh!

Document FQCN named controllers#7771

Document FQCN named controllers#7771

Uh oh!

Conversation

GuilhemN commentedApr 8, 2017• editedLoading 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!

GuilhemNApr 8, 2017• editedLoading 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!

HeahDudeApr 12, 2017• editedLoading 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!

GuilhemNApr 12, 2017• editedLoading 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 commentedMay 2, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

javiereguiluz commentedMay 3, 2017

Uh oh!

GuilhemN commentedMay 3, 2017

Uh oh!

javiereguiluz commentedMay 3, 2017

Uh oh!

GuilhemN commentedMay 3, 2017

Uh oh!

xabbuh commentedMay 4, 2017

Uh oh!

GuilhemN commentedMay 4, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

GuilhemN commentedMay 5, 2017

Uh oh!

GuilhemN commentedMay 10, 2017

Uh oh!

robfrawley commentedMay 11, 2017

Uh oh!

GuilhemN commentedMay 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

GuilhemN commentedApr 8, 2017•
edited
Loading

GuilhemNApr 8, 2017•
edited
Loading

HeahDudeApr 12, 2017•
edited
Loading

GuilhemNApr 12, 2017•
edited
Loading

weaverryan commentedMay 2, 2017•
edited
Loading

GuilhemN commentedMay 4, 2017•
edited
Loading