laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.68k stars 11.05k forks source link

Using url helper in config files breaks Artisan #7671

Closed martinbean closed 9 years ago

martinbean commented 9 years ago

Implementing Socialite, I decided to use the url() helper to generate my absolute redirect URLs, for example:

return [
    'facebook' => [
        'redirect' => url('auth/redirect/facebook'),
    ],
];

Although the HTTP application continues to work normally, I’ve noticed that any subsequent Artisan commands fail with the following RuntimeException:

Error Output: PHP Catchable fatal error: Argument 2 passed to Illuminate\Routing\UrlGenerator::__construct() must be an instance of Illuminate\Http\Request, null given, called in /home/vagrant/Sites/southsidewrestling.co.uk/vendor/laravel/framework/src/Illuminate/Routing/RoutingServiceProvider.php on line 56 and defined in /home/vagrant/Sites/southsidewrestling.co.uk/vendor/laravel/framework/src/Illuminate/Routing/UrlGenerator.php on line 81

This also occurs if I try to resolve the helper in its “longhand” form:

return [
    'facebook' => [
        'redirect' => app('url')->to('auth/redirect/facebook'),
    ],
];
arrilot commented 9 years ago

Configs are loaded really early and probably not meant to use anything from the framework except Dotenv.

GrahamCampbell commented 9 years ago

Yeh, you can't do that. You should only use plain strings, etc in the config. If you config is dynamic, you're missusing it.

martinbean commented 9 years ago

@grahamcampbell I want my OAuth redirect URLs to be based on my app’s current domain. That seems a pretty standard use case to me?

GrahamCampbell commented 9 years ago

Nope.

GrahamCampbell commented 9 years ago

Take a look at how I do this: https://github.com/StyleCI/StyleCI/blob/master/config/services.php#L50.

montogeek commented 9 years ago

Much wisdom.

crynobone commented 9 years ago

Could as well utilize the App\Providers\ConfigServiceProvider.

GrahamCampbell commented 9 years ago

Could as well utilize the App\Providers\ConfigServiceProvider.

Yeh, very true. What @crynobone is suggesting here is that if you're existing services don't immediately depend on the config (ie, they're always instantiated after the config service provider has booted), then you are free to call the url classes within the boot method of that service provider.

bbatsche commented 9 years ago

Having been bitten by this exact same issue, I'd like to throw my own 2¢ in.

I think having to explicitly define an application's URL in the .env introduces unnecessary extra onus on users of the framework, and therefore greater opportunity for errors. Following your advice, my application's base URL could be three different values defined in three different places:

Clearly the detected value should take precedence, and it does under most circumstances. But by explicitly using APP_URL in services.php I have created an instance where, regardless of what URL my application is actually running under, a hard coded value will be used instead. Granted, in this example if my application's URL is different than what the OAuth provider is configured to expect I will be getting an error no matter what. But the error should be relating to URL mismatch, not because I'm being redirected to a domain name different than my application's.

Furthermore, looking through the documentation on helper functions I see url(), route(), and env() all defined as peers. How am I to know that I can use one of these in some contexts but not the others? And why should all of them work when the request is web based, but not from the command line? Yes artisan doesn't handle web based requests, but from reading the comments in config/app.php, that's why there is that url key in there.

Finally, a bit of a philosophical question that is certainly beyond the scope of this topic but makes me curious. I absolutely agree that configuration files should not be dynamic. But if that's the case, why are these config files PHP, and not something like Yaml or JSON? Why even introduce the opportunity for extra syntax and logic in these files in the first place?

</rant>

All that aside, @GrahamCampbell, I do appreciate your help in explaining a workaround. I just disagree on whether that workaround should be necessary. Thanks!

martinbean commented 9 years ago

Following @crynobone, I implemented this in an app today by setting the value in my ConfigServiceProvider as follows:

public function register()
{
    config([
        'services.facebook.redirect' => url('auth/callback/facebook'),
    ]);
}
pun-ky commented 9 years ago

I am designing multi-package application and each of package has own rules which determine where redirect after auth. This static config value makes ma a lot of troubles...

Why we just cannot add extra parameter to method:

$provider->redirect('/my/overidden/url/to/redirect')

?

BrandonSurowiec commented 9 years ago

@martinbean Thank you for the idea to transform the urls in the service provider. @pun-ky I put partial urls in my config 'redirect' => 'auth/call/this' and then I transform them in ConfigServiceProvider:

<?php

namespace App\Providers;

use Illuminate\Support\ServiceProvider;
use Illuminate\Support\Facades\Config;

class ConfigServiceProvider extends ServiceProvider
{
    /**
     * Register the application services.
     *
     * @return void
     */
    public function register()
    {
        $this->makeAbsoluteUrls();
    }

    /**
     * Make relative urls into absolute urls
     *
     * @return void
     */
    public function makeAbsoluteUrls()
    {
        foreach(Config::get('services') as $key => $config) {

            if ( ! isset($config['redirect'])) continue;

            Config::set("services.$key.redirect", url($config['redirect']));
        }
    }
}
pcsegal commented 7 years ago

@GrahamCampbell

Take a look at how I do this: https://github.com/StyleCI/StyleCI/blob/master/config/services.php#L50.

The link is broken. Could you provide an alternative link for the example? Thank you in advance.

shahrukh4 commented 6 years ago

I figured out the problem, when you are running any artisan command you should avoid using helper functions in any of your config files. Just comment those and try to run artisan command after running that uncomment your config files.

//in config/'any_file.php'
return [
   'name'   => 'Larvel',
   'url'    => url('/')
];

//just change and uncomment url() helper
return [
   'name'   => 'Larvel',
    //'url' => url('/)
];
martinbean commented 6 years ago

@shahrukh4 That approach doesn’t really work on staging/production servers though where you can’t edit code, does it?

sandervanhooft commented 5 years ago

I was having the same issue using route() in the config files.

Ended up switching to url(), and use this with the config() when resolving. This way I can set either a relative or absolute url in the config.

Before (ERRORS!):

// config file
'webhook_url' => route('webhooks.mollie.default'),
// consumption
$url = config('somepackage.webhook_url'); // AARGH

After (all good 🌴):

// config file
'webhook_url' => 'webhooks/mollie', // relative
'webhook_url' => 'https://awesomecorp.com/webhooks/mollie', // absolute also works
// consumption
$url = url(config('somepackage.webhook_url`));

BTW: Hardcoding a value in a (config) service provider using config(['key' => 'value']) completely negates the benefits of a configuration for me. I'd rather use a constant in a dedicated type class.

Tip: use config(['key' => 'value']) only for testing purposes.

devcircus commented 5 years ago

No reason to hardcode values, you just shouldn't use laravel dependencies inside config because the config is loaded early in the request life cycle and things like global helpers may not be available.

amitshahc commented 5 years ago

I solved this by this: Put all dynamic values into the .env `

'google' => [ 'client_id' => env('AUTH_GOOGLE_CLIENT_ID'), 'client_secret' => env('AUTH_GOOGLE_CLIENT_SECRET'), 'redirect' => env('APP_URL').'/auth/google/callback', ]`

martinbean commented 5 years ago

@amitshahc This has long been solved. Relative URLs in config files will be expanded to the absolute URL.

From https://laravel.com/docs/5.8/socialite#configuration:

If the redirect option contains a relative path, it will automatically be resolved to a fully qualified URL.

amitshahc commented 5 years ago

@amitshahc This has long been solved. Relative URLs in config files will be expanded to the absolute URL.

From https://laravel.com/docs/5.8/socialite#configuration:

If the redirect option contains a relative path, it will automatically be resolved to a fully qualified URL.

yeh but i am using 5.5 which has this issue. so for those using earlier versions can have this solution.