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

Prevent GeocoderService from instantiating providers on boot #130

Conversation

Korri
Copy link
Contributor

@Korri Korri commented Jun 22, 2018

The current code creates an instance of each configured provider directly in the boot method (side effect of registerProvidersFromConfig), this is not good practice as most likely this library won't be used on every request.

In my case it would cause a bigger issue as we do composer install before having a .env file created, and this issue would cause artisan discover to fail as it was trying to instantiate some providers without an API key.

This could cause performance issues as is instantiates the each provider that was setup in the configuration as the time of boot, event if they don't get use.
@Korri Korri force-pushed the no-instantiation-on-boot branch from 1a484b2 to 6637132 Compare June 22, 2018 18:02
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 91.818% when pulling 6637132 on Korri:no-instantiation-on-boot into fd0c837 on geocoder-php:master.

@mikebronner mikebronner self-assigned this Jun 22, 2018
@mikebronner mikebronner self-requested a review June 22, 2018 18:33
@mikebronner
Copy link
Member

Hi @Korri Thanks for submitting this PR. I will take a look at this over the next few weeks. I won't have time this weekend, but hopefully by next weekend.

@mikebronner mikebronner changed the base branch from master to feature/optimize-service-provider July 1, 2018 18:07
@mikebronner mikebronner merged commit 615c046 into geocoder-php:feature/optimize-service-provider Jul 1, 2018
@Korri Korri deleted the no-instantiation-on-boot branch August 9, 2018 17:50
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.

3 participants