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

route() helper function does not work well in artisan commands #14139

Closed
huynhducduy opened this issue Jun 26, 2016 · 28 comments
Closed

route() helper function does not work well in artisan commands #14139

huynhducduy opened this issue Jun 26, 2016 · 28 comments
Labels

Comments

@huynhducduy
Copy link

In Laravel 5.2.39, my app has

.env

APP_URL=http://dev.env/smartbots

config/app.php

'url' => env('APP_URL'),

app/Http/routes.php

Route::get('test', function () {
    dd(route('twilioVoiceRequest'));
});

Route::get('/twilio-voice-request', function() {
})->name('twilioVoiceRequest');

Artisan command (twilio:setup)

public function handle()
{
    dd(route('twilioVoiceRequest'));
}

This is what i got from test route
untitled

and from php artisan twilio:setup
untitled2

@GrahamCampbell
Copy link
Member

Try adding a trailing slash to the URL and/or running php artisan config:cache.

@huynhducduy
Copy link
Author

i tried it before, still the same result

@GrahamCampbell
Copy link
Member

This is definitely due to an out of date config cache otherwise the cli would have just generated localhost.

@crynobone
Copy link
Member

APP URL only work on domain level, right?

@GrahamCampbell
Copy link
Member

It should be defining the base URI?

@GrahamCampbell
Copy link
Member

We definitely don't recommend you run apps on sub-folders though.

@crynobone
Copy link
Member

crynobone commented Jun 26, 2016

route() helper use toRoute() and getRouteDomain() https://github.com/laravel/framework/blob/5.2/src/Illuminate/Routing/UrlGenerator.php#L324

Is this a bug?

@GrahamCampbell
Copy link
Member

Yeh, I'd say so.

@findelallc
Copy link

With support @GrahamCampbell I would suggest, leave a slash at trail,laravel will auto handle your route.

@j3j5
Copy link
Contributor

j3j5 commented Sep 22, 2016

Changing $this->request->root() with $this->request->url() here seems to fix this issue.

Would that be an acceptable solution for you @GrahamCampbell ? I've run the unit tests and nothing seems to break but I didn't have time yet to check if this part is covered on the tests yet.

@j3j5
Copy link
Contributor

j3j5 commented Sep 22, 2016

Ok, after further testing the above is not the solution when not using the console. We'll have to find a different fix (and ideally add some test to cover this code).

@j3j5
Copy link
Contributor

j3j5 commented Sep 26, 2016

The problem seems to come all the way from Symfony\Component\HttpFoundation\SymfonyRequest, in order to get the baseUrl of the request, SymfonyRequest compares the values of $_SERVER['SCRIPT_NAME'] and $_SERVER['SCRIPT_FILENAME'], and those aren't being set by Illuminate\Foundation\Bootstrap\SetRequestForConsole.

@taylorotwell
Copy link
Member

I can't recreate this issue: http://d.pr/i/1bqOj

@j3j5
Copy link
Contributor

j3j5 commented Sep 28, 2016

I don't think you understood well what the issue is.

When the config value of app.url is set ot something else than a base domain (like http://some-random-url.com/somethingelse/), all the routes generated from console by the UrlGenerator remove the path. On the other hand, if your app is set to be accessed through that same path, the UrlGenerator creates correct paths.

Now, in order to recreate it you'll have to add some path to your base url.

@engAhmad
Copy link

engAhmad commented Oct 6, 2016

config('app.url');
using the config helper function works fine

I had this issue with the Queues The url helper returns http://localhost

@petar-dambovaliev
Copy link

@engAhmad , if you don't have http[s]?:// prefix to your environment url laravel will not detect it and overwrite it with http://localhost.

@vlakoff
Copy link
Contributor

vlakoff commented Nov 25, 2016

Until the situation is improved, as a workaround you could add this line early:

url()->forceRootUrl(config('app.url'));

@vlakoff
Copy link
Contributor

vlakoff commented Nov 25, 2016

Basically the autodetection doesn't work in CLI, and we do have to pass the base URL to the UrlGenerator. I'm posting a few suggestions in #15608.

@amosmos
Copy link

amosmos commented Jan 18, 2017

I think the reason because some functions use the root as it is calculated by symphony and not based on the env setting.

I think the reason is this line: https://github.com/laravel/framework/blob/5.3/src/Illuminate/Http/Request.php#L87

@vlakoff
Copy link
Contributor

vlakoff commented Jan 18, 2017

Hence the 2 possible fixes I posted in #15608, which make use of forcedRoot in UrlGenerator.

We are in CLI so it's normal the request doesn't have an URL. Modifying the request doesn't sound right, while using forcedRoot of the UrlGenerator seems much more appropriate.

@amosmos
Copy link

amosmos commented Jan 18, 2017

You might be right but I ran into this issue in more cases, not in CLI but in the web (actually on local development on homestead). I just think that if we have an APP_URL environment variable, we should use it - no?

@vlakoff
Copy link
Contributor

vlakoff commented Jan 18, 2017

I can't say for homestead… Let's reference issue #17385 you have just created :)

@amosmos
Copy link

amosmos commented Jan 18, 2017

Thanks.

But what do you think about the general idea of: we already have an APP_URL environment variable, why isn't the url helper use it?

@vlakoff
Copy link
Contributor

vlakoff commented Jan 18, 2017

I'd say because it is not configured by default, so using it would require an extra step in new installs, while the autodetection works in most cases.

Also, the autodetection is convenient when the app can be accessed through several domains ;) Don't have to dynamically configure the path.

@amosmos
Copy link

amosmos commented Jan 18, 2017

Yes you are correct. But in that case I think that A) Why is the APP_URL there and why the laravel config uses it? and B) The auto detection should work flawlessly... at least for web requests...

@j3j5
Copy link
Contributor

j3j5 commented Jan 18, 2017

@amosmos The APP_URL is there to simulate requests only when using the console, it is actually explained on the (config file itself)[https://github.com/laravel/laravel/blob/master/config/app.php#L48].
I've never experienced the issue with a web requests, having the installation on a subpath always worked flawlessly for me. Can you better describe your issue with web requests?

@mtx-z
Copy link

mtx-z commented Mar 20, 2017

I had the exact same issue running a Artisan command that should generate my sitemap.xml.
All the generation was Route based to get routes URLs (and LaravelLocalization for translated routes URLs).
What I had was wrong generated URL with http://localhost instead of full URL path (wich was correct when getting the same method from a HTTP request).

So, in my command __construct() method, i added:

!App::runningInConsole() ?: \URL::forceRootUrl(\Config::get('app.url'));

And URLs generation methods used in my Artisan command was based on the app.url value, same value as when fired from a HTTP request on a route. Maybe it's a way to force the URL for specific localy run methods, with Artisan or else.
(tested on L5.1, Wamp with full URL as http://localhost/project_name/public/, error URL from Artisan command was http://localhost/, app.url as http://localhost/project_name/public/ - result for the same method fired from a Route GET or a Artisan command).

Links: pull #15608, stack

@aik099
Copy link
Contributor

aik099 commented Aug 18, 2017

I've looked into Symfony to see how they've solved the problem, when route URL is build from CLI and website is located at sub-path and found this:

It works like this:

  1. in the app/config/parameters.yml file you specify 3 settings: host, scheme, base_url (basically what can't be detected in CLI)
  2. the RequestContext class is created using these settings (see https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Resources/config/routing.xml#L81-L88)
  3. when UrlMatcher is building route url it's using data from context (see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Routing/Matcher/UrlMatcher.php#L257-L260) and also replacing SCRIPT_FILENAME and SCRIPT_NAME server attributes

Based on that info and @j3j5 comment (see #14139 (comment)) I'm suggesting to change \Illuminate\Foundation\Bootstrap\SetRequestForConsole::bootstrap method like so:

  1. parse url in app.url config setting using parse_url function (don't use Symfony's Request because we're using it already and it fails in CLI)
  2. if the path component is found, then replace SCRIPT_NAME and SCRIPT_FILENAME in server array passed to Request creation later

Code before:

$app->instance('request', Request::create(
    $app->make('config')->get('app.url', 'http://localhost'), 'GET', [], [], [], $_SERVER
));

Code after:

$uri = $app->make('config')->get('app.url', 'http://localhost');
$components = parse_url($uri);
$server = $_SERVER;

if (isset($components['path'])) {
    $server['SCRIPT_FILENAME'] = $components['path'];
    $server['SCRIPT_NAME'] = $components['path'];
}

$app->instance('request', Request::create($uri, 'GET', [], [], [], $server));

I can send a PR if you'd like as a bugfix.

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

No branches or pull requests