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

[5.6] Fix relative route URL generation with custom host formatter #24051

Merged
merged 2 commits into from
Apr 29, 2018
Merged

[5.6] Fix relative route URL generation with custom host formatter #24051

merged 2 commits into from
Apr 29, 2018

Conversation

staudenmeir
Copy link
Contributor

Generating relative route URLs doesn't work with a custom host formatter:

$url = new UrlGenerator(
    $routes = new RouteCollection,
    $request = Request::create('http://www.foo.com/')
);

$routes->add(
    new Route(['GET'], '/named-route', ['as' => 'plain'])
);

$url->formatHostUsing(function ($host) {
    return str_replace('foo.com', 'foo.org', $host);
});

dd($url->route('plain', [], false));
// expected: '/named-route'
// actual: '/http://www.foo.org/named-route'

RouteUrlGenerator::to() tries to remove the original root, but fails because the root has been modified.

Fixes #24047.

@taylorotwell taylorotwell merged commit 6656ed6 into laravel:5.6 Apr 29, 2018
@vlakoff
Copy link
Contributor

vlakoff commented Apr 30, 2018

That's an interesting regex… I tried this and it seems to work fine, but maybe there are some unit tests to add (for example, with a query string).

@staudenmeir
Copy link
Contributor Author

There are a lot of tests in RoutingUrlGeneratorTest::testBasicRouteGeneration().

@staudenmeir staudenmeir deleted the relative-url branch April 30, 2018 02:06
@vlakoff
Copy link
Contributor

vlakoff commented Apr 30, 2018

Sure thing, thanks for pointing these. Apparently what I had in mind is already covered.

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

Successfully merging this pull request may close these issues.

3 participants