Skip to content

Resolve Geocoder dependency via Class name #125

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

Merged
merged 2 commits into from
May 28, 2018

Conversation

anam-hossain
Copy link
Contributor

The PR will enable resolving dependency via class name. This is a minor fix and will not break any existing code.

At the moment, app('geocoder') is the only way to resolve the geocoder dependency and does not support dependency injection via controller.

The PR will allow the following:

namespace App\Http\Controllers;

use Geocoder\Laravel\ProviderAndDumperAggregator as Geocoder;

class GeocoderController extends Controller
{
    /**
     * Create a new controller instance.
     *
     * @return void
     */
    public function __construct(Geocoder $geocoder)
    {
         $this->geocoder = $geocoder;
    }

    public function index(Geocoder $geocoder)
    {
        //
    }
}

The PR also tests friendy as it will allow developer to swap dependency when needed.

$mock = \Mockery::mock(ProviderAndDumperAggregator::class);

$this->app->instance(ProviderAndDumperAggregator::class, $mock);

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 91.379% when pulling acaccc0 on anam-hossain:master into 9b899cf on geocoder-php:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 91.379% when pulling acaccc0 on anam-hossain:master into 9b899cf on geocoder-php:master.

@mikebronner
Copy link
Member

Hi @anam-hossain , thank you for submitting this. I will take a look at it, but it might take a week or two, as I won't be available this weekend because of holidays.

@sololance
Copy link

Any news on this? Thank you!

@mikebronner
Copy link
Member

@sololance Sorry for the delay. Taking a look at it now.

@mikebronner mikebronner merged commit eabbf2a into geocoder-php:master May 28, 2018
@sololance
Copy link

Looks like this doesn't work perfectly.

>>> use Geocoder\Laravel\ProviderAndDumperAggregator as Geocoder;
>>> $geocoder = new Geocoder;
=> Geocoder\Laravel\ProviderAndDumperAggregator {#2547}
>>> $geocoder->reverse(43.882587,-103.454067)->get();
Geocoder/Exception/ProviderNotRegistered with message 'No provider registered.'

However this does work:

app('geocoder')->reverse(43.882587,-103.454067)->get();
=> Illuminate\Support\Collection {#2628
all: [
Geocoder\Provider\GoogleMaps\Model\GoogleAddress {#2509},
Geocoder\Provider\GoogleMaps\Model\GoogleAddress {#2561},
Geocoder\Provider\GoogleMaps\Model\GoogleAddress {#2582},
Geocoder\Provider\GoogleMaps\Model\GoogleAddress {#2568},
Geocoder\Provider\GoogleMaps\Model\GoogleAddress {#2569},
],
}
`

@mikebronner
Copy link
Member

mikebronner commented May 30, 2018

@sololance Please provide a complete code snippet and stack trace of the error in a new issue, as this PR is already merged. :) Thanks!

@sololance
Copy link

@mikebronner well, this only doesn't work in laravel tinker, anyways thank you ;)

@mikebronner
Copy link
Member

But it works when you use it in an app? I'm unclear now if you have a problem or not?

@sololance
Copy link

@mikebronner exactly, it does work when called in controllers, however doesn't work when called in laravel tinker.

I'm using tinker to see what responses are returned and handle them properly before writing some code in controllers.

@mikebronner
Copy link
Member

Thanks for clarifying. Yea, tinker is a bit special, no guarantees there. :)

@anam-hossain
Copy link
Contributor Author

@sololance The reason it's not working because you are trying to create a new instance of ProviderAndDumperAggregator and this is not related to this PR. ProviderAndDumperAggregator requires some setup to work. e.g. you need to setup config.

The following should work:

       $providerAndDumperAggregator = (new ProviderAndDumperAggregator)
            ->registerProvidersFromConfig(collect(config("geocoder.providers")));

Or if you want to avoid the above settings, use Laravel container to retrieve the geocoder instance:

use Geocoder\Laravel\ProviderAndDumperAggregator as Geocoder;
$geocoder = app(Geocoder::class);

@mikebronner
Copy link
Member

Thanks @anam-hossain ! So while you're already retrieving it from the IoC, you might as well use the original app("geocoder") syntax when tinkering.

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

Successfully merging this pull request may close these issues.

4 participants