silexphp / Silex

[DEPRECATED -- Use Symfony instead] The PHP micro-framework based on the Symfony Components
https://silex.symfony.com
MIT License
3.58k stars 718 forks source link

Reusing applications makes redirects awkward #98

Closed awildeep closed 13 years ago

awildeep commented 13 years ago

If I make an application and mount it to another application with a prefix, any self referencing redirects used get wonky.

For example: https://gist.github.com/984253

I understand the reason this fails as redirects do not care about the application context.

However if reusable applications are a goal of Silex I think being able to handle redirects in a way that could allow them to be modified would be ideal. Perhaps maybe we could have a method of redirection to a named route (this does not allow for variable passing however)?

Thoughts? Or am I missing something REALLY obvious?

The example I made is VERY simple implementation and not a good use of the framework but I wanted to demonstrate the awkwardness as simply as possible.

igorw commented 13 years ago

Okay, so I've run into this myself. In fact it was one of the main motivations behind "base_path parameter?".

What we could do is add a second $relative argument to redirect() that defaults to false. Why default to false? Because if you use UrlGeneratorExtension you don't want relative URLs, and we should prevent WTFs there.

return $app->redirect('/some/path', true);

Of course it's still kind of verbose:

return $app->redirect($app['url_generator']->generate('route_name'));

Suggestions welcome. :)

awildeep commented 13 years ago

base_path does not seem bad, just not very intuitive.

I was actually leaning toward a solution with named routes somehow, maybe a redirect for named routes that could take variables as an array?

stof commented 13 years ago

@awildeep The issue is that routes are available only when you use the UrlGeneratorExtension, so this cannot be done in the core (the extension is optionnal). The redirection to an URL needs to be fixed when mounting apps or using an app outside the root of the domain.

awildeep commented 13 years ago

Stof, I agree. I don't think either solution is perfect. On the bright side UrlGeneratorExtension works, without any modifications to the core now. (test it with: https://gist.github.com/984722)

On base_path, would this be configured auto-magically by the mount() method?

Also what happens when a user manually changes base_path (You know it will happen eventually)?

igorw commented 13 years ago

My use case for base_path was not mounted applications. I'm unsure if there is a simple and clean solution with base_path.

The cleanest way right now are named routes with UrlGenerator.

stof commented 13 years ago

@awildep I know that named route work. The point is finding a solution for people not using this extension (and for the case where the app is not at the root of the domain for the issue faced by @igorw)

Ziumin commented 13 years ago

my idea is to save prefix to $app in mountHandler and modify Application::redirect to use it if route is not like http:// or so..

igorw commented 13 years ago

I'm really not a fan of that kind of magic behaviour, imo it should be an argument if we go that route (no pun intended).

awildeep commented 13 years ago

@stof; we are in agreement. I only posted my example in case others were looking for a work-around now. Personally I am okay with enabling the UrlGeneratorExtension in my current application dev, but it would be great to have a way to deal with this without the extension.

What about leaving redirect() as-is, and adding a method like relativeRedirect() that could prepend the value from the parental mount methods (in theory, apps could be mounted multiple layers deep)?

Ziumin commented 13 years ago

Good idea, @awildeep. one redirect method which takes a route as parameter, another one - which takes a path.

igorw commented 13 years ago

That however will make the core depend on the UrlGenerator, so there is a bigger discussion in there.

awildeep commented 13 years ago

@igorw, I am not sure that it does. Assuming you have access to the mount prefix(es) inside of a relativeRedirect() method, you could just prepend them to the string.

For instance:

$admin->get('/', function () use ($admin) {
    $admin->relativeRedirect('/dashboard');
});

$app->mount('/admin', $admin);

If in this case the relativeRedirect() could see a string (say base_path) containing '/admin', it could then prepend its redirect. Also having this as a different method means that we can document it well, and hopefully have less headache from improper usage of redirect().

The beauty of this is that I could mount the $admin app to anything, and not have to change my admin application at all. Or I could reuse an application multiple times and have it mounted at different locations. The downside being a somewhat "magical" effect.

fabpot commented 13 years ago

If you are not using the UrlGenerator extension, you are on your own when dealing with URLs, and there is nothing we can do to "fix" them. The proper fix is to use the UrlGenerator extension.

igorw commented 13 years ago

Agree with this.