Closed albertovincenzi closed 3 years ago
Hey @albertovincenzi, thanks for the PR 😄
It seems like a lot of people have been trying to add this feature for a while and I keep rejecting it because I think it's a bit more complicated than it looks on the surface.
Hopefully, you can give me a bit of insight here. I'm trying to figure out how this might work with the ShortURLController
that comes with the package. Let's say that we have the ShortURLController
sat on the https://domain.com/short/{shortURLKey}
route.
If we now use your method to change the default_short_url
for a short URL to https://custom-domain/s/short-url-key
, how would you be able to get to the ShortURLController
?
Would you just expect to make a new route in your routes file to point to that controller? :)
I'm glad to contribute and thanks for implementing this very useful package. I think you are absolutely right: they can potentially say that the package has an issue but it's instead just a "configuration" issue. Your solution is "bulletproof" as you manage routes statically.
We might try to make the developer more aware of issues, adding a config parameter like allow_custom_short_url
.
In case this parameter will be set to true, they will be able to call setDefaultShortUrl()
, otherwise we will throw an exception.
In general, I might say that if they are willing to customize that behavior, they must accept to apply some extra configuration.
They can set custom routes this way:
Route::domain('http://custom-domain')->group(function () {
Route::get('/s/{shortURLKey}', '\AshAllenDesign\ShortURL\Controllers\ShortURLController');
});
Route::domain('http://custom-domain2')->group(function () {
Route::get('/abc/{shortURLKey}', '\AshAllenDesign\ShortURL\Controllers\ShortURLController');
});
Solved this, I might say that the only "wrong" behaviors is that any custom domain will be able to redirect any shortURLKey so customized short URL is more likely to be a cosmetic solution. If we want to introduce a true custom domain, we should implement a referral check, in request (using $request->header('referer')
) and a new db column to filter the request.
Hey @albertovincenzi, thanks for the kind words! 😄
I think I'm going to hold off on merging this PR. My main reasoning is that the behaviour of the default_short_url
was always intended to be defined by the package and not the developer. So, if a developer wants to create their own custom route, they can can do it as it shows in the docs (https://github.com/ash-jc-allen/short-url#custom-route).
Thanks for taking the time to make the PR though, I appreciate it!
This is a "drop-in" change so library works exactly as before but allows to customize the URL if required