Issue with config('short-url.default_url')

[johnpaulmedina]

I can make PR if needed but I think the logic on the default_url is a bit flawed.

Real world example for short urls: bitly.com => bit.ly google.com => goo.gl

I wanted to implement something like so: livingdonorproject.org => ldpl.ink

I configured Laravel to work with both domains and installed the Short URL package. What I am noticing is that if I set the prefix to null then what happens is the default routing for Laravel app is broken and being overloaded by the Short URL package.

For instance my /login path is returning 404.

REGARDLESS of me setting the default_url on the Builder on line 209 in the routes() method you should be setting the Routes::domain($url) by default. It should be utilizing either the config('short-url.default_url') or config('app.url').

This will allow the package to overload the default app.url routes and work as intended with prefixes or no prefix while also allowing a secondary domain that applies all the routes ONLY to that domain when using the config('short-url.default_url')

What are your thoughts on this?

[johnpaulmedina]

Also to mention I think this can address some of the reasons why disabling default routes even exists.

[ash-jc-allen]

Hey!

If I'm understanding your point correctly, this is something that someone else pointed out a while ago too. Admittedly, I don't think I documented this part of the feature too well, but you can disable the default route from being registered by setting the disable_default_route to true. You can then add \AshAllenDesign\ShortURL\Facades\ShortURL::routes(); to the bottom of your web.php routes file.

This could potentially solve your issue.

The "disabling default routes" feature is something that was added to the very first version of Short URL (before the recent default_url and prefix changes). It exists because most projects that are using this package will be okay with just using the provided default routing. But in some cases (such as yours), you might want to define your custom routes and/or handle the visits differently using your own controller. In this case, the default route can be disabled to prevent anyone going to the package's route rather than your own custom route.

Hopefully this helps? 🙂

[johnpaulmedina]

Actually it's a bit more than just the default routing.

Since Laravel supports multiple domains, when you are allowing for the overwrite on the config of the default url, it will break the routing.

As you pointed out, you could fix this issue by adding it to the bottom of the routes file. That behavior should work as intended.

However, if you use the multiple domains with Laravel and you overwrite the default app url, then there would be no need to move the routes to the bottom of the route file because your package can define '->domain($domain)' in the Builder class when defining routes.

It minimizes the additional config, replacing the routes, and allows for multiple domain support to create a true short url.

Does this make sense?

[johnpaulmedina]

Also the issue you are referring to is when the prefix of your package is set to 'null' because of the catch all routing.

[johnpaulmedina]

Let me get back home - I took my kids to Disney for their birthday - but I would like to submit a PR for your review, it won't change anything, but allow for multiple domains when overwriting the default app domain and setting the prefix to null so that this package will work out the box and no need to configure custom routes or moving the route method to the bottom of the routes file.

[ash-jc-allen]

I'd be happy to review a PR for this. But, in all honesty, I'm not too sure about this belonging in the package because I like to keep the package as lean as possible for me to maintain and keep on top of. So I think this might be better implemented on a project-by-project basis in the application's code using the custom routing feature.

However, if enough people want this added, this might be something that I'll consider in the future 😄