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

Confusing and undocumented if statement in route subscriber #86

Open
pfrenssen opened this issue Jun 5, 2019 · 0 comments
Open

Confusing and undocumented if statement in route subscriber #86

pfrenssen opened this issue Jun 5, 2019 · 0 comments
Labels
good first issue Good for newcomers invalid This doesn't seem right

Comments

@pfrenssen
Copy link
Member

While reviewing #85 I found a dubious section of code in RouteSubscriber::alterRoutes() and no comments are given on why this is needed:

    // Replace the core register route controller.
    $route = $collection->get('user.register');
    if ($route instanceof Route) {
      // ...
    }

Why is it here assumed that RouteCollection::get() would return objects which are not Route objects? If this can happen, why would we ignore this silently?

I see from the definition of \Symfony\Component\Routing\RouteCollection::get() that it only returns Route or NULL:

    /**
     * Gets a route by name.
     *
     * @param string $name The route name
     *
     * @return Route|null A Route instance or null when not found
     */
    public function get($name);

I'm not sure if something came up during testing that necessitated this if statement, but it seems more likely that this is just a mistake and the if statement can be simply removed. If is is necessary then we should have some clear documentation on why this can happen, and a test to prove that we handle the case correctly.

Ref. https://github.com/openeuropa/oe_authentication/blame/master/src/Routing/RouteSubscriber.php#L37

This entire class could use an overhaul by the way, there is almost no documentation, except for some comments that explain what the code is doing, but not why.

@pfrenssen pfrenssen added good first issue Good for newcomers invalid This doesn't seem right labels Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

1 participant