Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security] Deleting useless controller #17154

Closed
wants to merge 1 commit into from

Conversation

ThomasLandauer
Copy link
Contributor

As already pointed out by @linaori in #10423 (comment), it's quite useless to create an empty controller, just for the route. So the recommended way should be to add it in routes.php - as with YAML and XML.

@carsonbot carsonbot added this to the 6.0 milestone Aug 10, 2022
@ThomasLandauer
Copy link
Contributor Author

@javiereguiluz javiereguiluz requested a review from wouterj August 11, 2022 10:55
@carsonbot carsonbot changed the title Deleting useless controller [Security] Deleting useless controller Aug 11, 2022
@gnito-org
Copy link
Contributor

gnito-org commented Aug 12, 2022

I feel this is unnecessarily opinionated. Creating a controller for just a route is a valid solution. Perhaps you could just add a note to say that it is not the most optimal solution.

One of the outstanding strengths of Symfony is the ability to accomplish the same thing in many different ways.

@xabbuh
Copy link
Member

xabbuh commented Aug 12, 2022

I do not really see why we should remove this example. If any dev want's to be completely consistent throughout there project, they will probably be interested in this code tab. We can however add a comment like this inside the method:

// This controller will never be called. Use any of the other config formats to configure the route without the need to create an empty controller.

@ThomasLandauer
Copy link
Contributor Author

@javiereguiluz Do you know why there's no "PHP" tab in the second code sample at https://symfony.com/doc/6.0/security.html#logging-out, even though it's present in the source code (line 1680)?:

    ..  code-block:: php

        // config/routes.php
        use Symfony\Component\Routing\Loader\Configurator\RoutingConfigurator;

        return function (RoutingConfigurator $routes) {
            $routes->add('app_logout', '/logout')
                ->methods(['GET'])
            ;
        };

@alexislefebvre
Copy link
Contributor

Maybe the double space is causing the issue:

.. code-block:: php

@ThomasLandauer
Copy link
Contributor Author

Guys, please merge #17166 first, so we can see if this works, then continue here...

xabbuh added a commit that referenced this pull request Aug 15, 2022
This PR was merged into the 5.4 branch.

Discussion
----------

Trying to fix code sample syntax

Thanks to  `@alexislefebvre` at #17154 (comment)

Commits
-------

7261de9 Trying to fix code sample syntax
@xabbuh
Copy link
Member

xabbuh commented Aug 15, 2022

#17166 seems to have fixed the display issue

@ThomasLandauer
Copy link
Contributor Author

That's OK for me now, since config/routes.php is shown as equal alternative. So I'm closing this.

@ThomasLandauer ThomasLandauer deleted the patch-29 branch August 15, 2022 13:26
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.

6 participants