Skip to content

Conversation

@theofidry
Copy link

Provides use Ivory adapters for executing the requests. Executing such requests may results in failures, which are currently thrown as Ivory\HttpAdapter\HttpAdapterException. As those exceptions are not catched, the user may get one of those issues instead of the expected Geocoder\Exception\Exception.

  • Provide a helper method in AbstractHttpProvider to safely execute the query via the adpater and retrieve the content
  • Add tests for each concerned adapters

Note: I'm not a big fan of protected methods, but in this case this proves to be the most effective way to deal with this issue.

Closes #497

@willdurand I'm not sure how you want to handle this patch, it could be backported to the previous 3.x releases but I know this can be a bit of a pain as well.

Provides use Ivory adapters for executing the requests. Executing such requests may results in failures, which are currently thrown as `Ivory\HttpAdapter\HttpAdapterException`. As those exceptions are not catched, the user may get one of those issues instead of the expected `Geocoder\Exception\Exception`.

- Provide a helper method in `AbstractHttpProvider` to safely execute the query via the adpater and retrieve the content
- Add tests for each concerned adapters

Closes geocoder-php#497
@Nyholm
Copy link
Member

Nyholm commented Dec 22, 2016

I like this PR and I'll be happy to merge it... But.. We do not use Ivory anymore. We use HTTPlug. Can you rebase and update this PR?

@Nyholm Nyholm added this to the 4.0.0 milestone Dec 22, 2016
Copy link

@imerkle imerkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@theofidry
Copy link
Author

@Nyholm will try next week

@imerkle
Copy link

imerkle commented Dec 29, 2016

why its failing fix it i need to use it

@theofidry
Copy link
Author

@dsslimshaddy please stop, you are being incredibly rude by behaving like that. If you need it urgently, fix it yourself and @Nyholm will gladly merge it ASAP. If you're not ready to do so fix it in your vendors or in a fork and be patient.

@theofidry
Copy link
Author

@Nyholm I'm a bit overwhelmed at work this month, so I probably won't be able to look at it before mid-February :/

@danijelk
Copy link

@theofidry don't forget this one please :)

@theofidry
Copy link
Author

@danijelk you can also take over :) I'm a bit too busy with work, Alice and Humbug lately :/

@Nyholm
Copy link
Member

Nyholm commented May 17, 2017

Have a look at #617. With that PR it will be easier to implement this feature.

@Nyholm
Copy link
Member

Nyholm commented May 20, 2017

Closed by #617

@Nyholm Nyholm closed this May 20, 2017
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