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

[Routing] Ability to exclude a route from matcher #23093

Closed
MatTheCat opened this Issue Jun 7, 2017 · 13 comments

Comments

Projects
None yet
7 participants
@MatTheCat
Contributor

MatTheCat commented Jun 7, 2017

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? no
Symfony version N/A

In an application I must build URL to another host. The host is environment-dependent and the path contains a parameter so I thought of using the router. It works great but my only concern is that the route will be in the UrlMatcher whereas it will never match. Would it be possible to add an option specifying whether a route can be matched or not?

@sstok

This comment has been minimized.

Show comment
Hide comment
@sstok

sstok Jun 8, 2017

Contributor

Unless you have route patterns that overlap (news/{title} with title=".+", which also matches news/1/edit) this shouldn't be a problem as /news/{id}/edit doesn't match /news/100, all routes are matched in full and unless you your requirement is "greedy" (matches beyond the / delimiter) you wont have this problem.

But do note that you control the requirement (a regex) and you can use a negative look-ahead to prevent matching, it's only a bit tricky to get this right as you only want to prevent conflicts while still allowing other matches.

You can have a look at https://github.com/rollerworks/app-sectioning-bundle which helps with separating the application into back-end/frontend sections. It's in alpha state but maybe some of it's logic can help you :)

Contributor

sstok commented Jun 8, 2017

Unless you have route patterns that overlap (news/{title} with title=".+", which also matches news/1/edit) this shouldn't be a problem as /news/{id}/edit doesn't match /news/100, all routes are matched in full and unless you your requirement is "greedy" (matches beyond the / delimiter) you wont have this problem.

But do note that you control the requirement (a regex) and you can use a negative look-ahead to prevent matching, it's only a bit tricky to get this right as you only want to prevent conflicts while still allowing other matches.

You can have a look at https://github.com/rollerworks/app-sectioning-bundle which helps with separating the application into back-end/frontend sections. It's in alpha state but maybe some of it's logic can help you :)

@MatTheCat

This comment has been minimized.

Show comment
Hide comment
@MatTheCat

MatTheCat Jun 8, 2017

Contributor

I think you misunderstood: host is different so the route will never match. But the matcher will check for it anyway, which is pointless. So I ask for a way to define a route which will not be checked by the matcher.

Contributor

MatTheCat commented Jun 8, 2017

I think you misunderstood: host is different so the route will never match. But the matcher will check for it anyway, which is pointless. So I ask for a way to define a route which will not be checked by the matcher.

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Jun 9, 2017

Contributor

I was thinking about this as well recently. We have a few URLs that are generated to link towards other hosts, but will never get matched. However, there are still rules created to match those routes, which simply can't be matched.

Contributor

iltar commented Jun 9, 2017

I was thinking about this as well recently. We have a few URLs that are generated to link towards other hosts, but will never get matched. However, there are still rules created to match those routes, which simply can't be matched.

@MatTheCat

This comment has been minimized.

Show comment
Hide comment
@MatTheCat

MatTheCat Jul 24, 2017

Contributor

Could we just filter $this->getRoutes() on a route option here?

Contributor

MatTheCat commented Jul 24, 2017

Could we just filter $this->getRoutes() on a route option here?

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Feb 21, 2018

Member

What would be the benefit of being able to exclude those routes. It they cannot match, it's just extra information that does not hurt.

Member

fabpot commented Feb 21, 2018

What would be the benefit of being able to exclude those routes. It they cannot match, it's just extra information that does not hurt.

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Feb 21, 2018

Contributor

@fabpot they actually can match, but aren't ever matched as long as the application routing is matched first. Example:

// somewhere above
if (...) {
    // ...
    // app.authentication.login
    if ('/login' === $trimmedPathinfo) {
        $ret = array (  '_controller' => 'Hostnet\\App\\Controller\\Authentication\\LoginController::loginAction',  '_route' => 'app.authentication.login',);
        if (substr($pathinfo, -1) !== '/') {
            return array_replace($ret, $this->redirect($rawPathinfo.'/', 'app.authentication.login'));
        }

        return $ret;
    }
    // ...
}

// some more routes

if (preg_match('#^cp\\.(?P<environment_url>([a-z]+\\.)?domain.nl)$#si', $host, $hostMatches)) {
    // ...
    // frontend_routing.cp.login_page
    if ('/login' === $trimmedPathinfo) {
        $ret = $this->mergeDefaults(array_replace($hostMatches, array('_route' => 'frontend_routing.cp.login_page')), array ());
        if (substr($pathinfo, -1) !== '/') {
            return array_replace($ret, $this->redirect($rawPathinfo.'/', 'frontend_routing.cp.login_page'));
        }

        return $ret;
    }
    // ...
}

This has some side effects that I rather see impossible:

  • If the order changes for whatever reason, things break, because in this application the host regex will match (shared bundle defining all cross-linking routes between apps).
  • As it can never be matched, it's only extra overhead in the matching for this host + unmatched routes, especially if you have fallback routes
Contributor

iltar commented Feb 21, 2018

@fabpot they actually can match, but aren't ever matched as long as the application routing is matched first. Example:

// somewhere above
if (...) {
    // ...
    // app.authentication.login
    if ('/login' === $trimmedPathinfo) {
        $ret = array (  '_controller' => 'Hostnet\\App\\Controller\\Authentication\\LoginController::loginAction',  '_route' => 'app.authentication.login',);
        if (substr($pathinfo, -1) !== '/') {
            return array_replace($ret, $this->redirect($rawPathinfo.'/', 'app.authentication.login'));
        }

        return $ret;
    }
    // ...
}

// some more routes

if (preg_match('#^cp\\.(?P<environment_url>([a-z]+\\.)?domain.nl)$#si', $host, $hostMatches)) {
    // ...
    // frontend_routing.cp.login_page
    if ('/login' === $trimmedPathinfo) {
        $ret = $this->mergeDefaults(array_replace($hostMatches, array('_route' => 'frontend_routing.cp.login_page')), array ());
        if (substr($pathinfo, -1) !== '/') {
            return array_replace($ret, $this->redirect($rawPathinfo.'/', 'frontend_routing.cp.login_page'));
        }

        return $ret;
    }
    // ...
}

This has some side effects that I rather see impossible:

  • If the order changes for whatever reason, things break, because in this application the host regex will match (shared bundle defining all cross-linking routes between apps).
  • As it can never be matched, it's only extra overhead in the matching for this host + unmatched routes, especially if you have fallback routes
@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Feb 21, 2018

Member

Is it reasonable to say that the "overhead" is not something we need to worry about? If yes, I would close this issue.

Member

fabpot commented Feb 21, 2018

Is it reasonable to say that the "overhead" is not something we need to worry about? If yes, I would close this issue.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Feb 21, 2018

Member

The overhead is negligible IMHO, the router handles any size of route collections in constant time. I wouldn't care for this aspect.

Member

nicolas-grekas commented Feb 21, 2018

The overhead is negligible IMHO, the router handles any size of route collections in constant time. I wouldn't care for this aspect.

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Feb 21, 2018

Contributor

Fair enough, especially after your recent additions!

What about the matching order though? Right now it's more of a "I found out we had this and luckily it didn't break", but it could be causing issues in the future. A test case that prevents this from happening would be enough to satisfy me to be honest.

Edit: Maybe we already have one?

Contributor

iltar commented Feb 21, 2018

Fair enough, especially after your recent additions!

What about the matching order though? Right now it's more of a "I found out we had this and luckily it didn't break", but it could be causing issues in the future. A test case that prevents this from happening would be enough to satisfy me to be honest.

Edit: Maybe we already have one?

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Feb 21, 2018

Member

If the host has a requirement that cannot match, then the route will never match. It does not matter if it's first or last in that regard.

Member

Tobion commented Feb 21, 2018

If the host has a requirement that cannot match, then the route will never match. It does not matter if it's first or last in that regard.

@Tobion Tobion closed this Feb 21, 2018

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Feb 22, 2018

Contributor

@Tobion except that it will match in my scenario as the host requirement is matching

Contributor

iltar commented Feb 22, 2018

@Tobion except that it will match in my scenario as the host requirement is matching

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Feb 22, 2018

Member

I don't get what you are asking for. Routes are matched in order and we have lots of tests for this.

Member

Tobion commented Feb 22, 2018

I don't get what you are asking for. Routes are matched in order and we have lots of tests for this.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Feb 23, 2018

Member

Since the generator doesn't care about the verb and the condition, we could easily add a special verb or condition that the matcher-dumper could use to skip generating code for those.
Could be e.g. methods: generate or condition: false
PR welcome suppose is any of you want to make it happen.

Member

nicolas-grekas commented Feb 23, 2018

Since the generator doesn't care about the verb and the condition, we could easily add a special verb or condition that the matcher-dumper could use to skip generating code for those.
Could be e.g. methods: generate or condition: false
PR welcome suppose is any of you want to make it happen.

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