Skip to content

Conversation

aschempp
Copy link
Member

I think the menu listener is invalid as well. @sheeep can you check if the code works?

@fritzmg fritzmg mentioned this pull request Jan 16, 2020
2 tasks
Copy link
Contributor

@fritzmg fritzmg left a comment

Choose a reason for hiding this comment

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

Thank you @aschempp :)

Imho we should advocate the "Action Domain Responder" (ADR) pattern, especially when using __invoke.

* @Route("/my-backend-route", name="app.backend-route")
* @Template("my_backend_route.html.twig")
* @Route("", name="app.backend-route")
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole annotation block is not needed anymore, is it?

Copy link
Contributor

Choose a reason for hiding this comment

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

This still needs to be removed.

* "_backend_module" = "my-modules"
* })
* @Route("/contao//my-backend-route",
* name="app.backend-route"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* name="app.backend-route"
* name=App\Action\BackendAction::class,

Copy link
Member Author

Choose a reason for hiding this comment

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

I have never seen a route with a class name, not sure this works (and is best practice?)

Copy link
Contributor

@fritzmg fritzmg Jan 16, 2020

Choose a reason for hiding this comment

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

Yes this works and is much more useful as you can then simply write

$this->router->generate(BackendAction::class)

in PHP.

Copy link
Member

Choose a reason for hiding this comment

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

There is a trailing comma missing after name="" which leads to AnnotationException.php line 42: [Syntax Error] Expected Doctrine\Common\Annotations\DocLexer::T_CLOSE_PARENTHESIS, got 'defaults' error

Co-Authored-By: Fritz Michael Gschwantner <[email protected]>
* "_backend_module" = "my-modules"
* })
* @Route("/contao/my-backend-route",
* name="app.backend-route"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* name="app.backend-route"
* name=App\Controller\BackendController::class

@aschempp
Copy link
Member Author

is there any recommendation from Symfony on how to name routes?
/cc @contao/developers

* "_backend_module" = "my-modules"
* })
* @Route("/contao//my-backend-route",
* name="app.backend-route"
Copy link
Member

Choose a reason for hiding this comment

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

There is a trailing comma missing after name="" which leads to AnnotationException.php line 42: [Syntax Error] Expected Doctrine\Common\Annotations\DocLexer::T_CLOSE_PARENTHESIS, got 'defaults' error

@richardhj
Copy link
Member

Currently getting error
The controller for URI "/contao/my-backend-route" is not callable. Controller "App\Controller\BackendController" cannot be fetched from the container because it is private. Did you forget to tag the service with "controller.service_arguments"?

We should add the following to the services.yml (in this document).

  App\Controller\:
    resource: ../src/Controller/*
    tags:
      - name: controller.service_arguments

WDYT @aschempp?

@aschempp
Copy link
Member Author

aschempp commented Feb 7, 2020

@richardhj that should not be necessary if your class extends the AbstractController and you have autoconfig enabled?

@fritzmg
Copy link
Contributor

fritzmg commented Feb 7, 2020

I personally prefer to not extend from the AbstractController and instead making the service simply public.

But since the example does extend, no services.yml change should be necessary (if autowire is enabled).

* @Route("/my-backend-route", name="app.backend-route")
* @Template("my_backend_route.html.twig")
* @Route("", name="app.backend-route")
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

This still needs to be removed.

@bytehead
Copy link
Member

bytehead commented Feb 7, 2020

Why do you not prefer to extend from the AbstractController?

@fritzmg
Copy link
Contributor

fritzmg commented Feb 7, 2020

Why do you not prefer to extend from the AbstractController?

I usually don't actually need any features from AbstractController.

@fritzmg fritzmg merged commit b74e349 into master Mar 4, 2020
@fritzmg fritzmg deleted the bugfix/backend-routes branch March 4, 2020 15:28
@fritzmg
Copy link
Contributor

fritzmg commented Mar 4, 2020

Thank you @aschempp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants