geocoder-php / geocoder-extra

Geocoder extra features.
MIT License
52 stars 13 forks source link

Fix broken unit tests for Baidu (via PR #33) #48

Closed vicchi closed 8 years ago

vicchi commented 8 years ago

As noted in https://github.com/geocoder-php/geocoder-extra/pull/33#issuecomment-253728021, the pull request as submitted broke the unit tests. This PR incorporates the code changes to BaiduProvider.php as well as fixing up the unit tests as a result of the changed endpoint URLs for v2 of Baidu's geocoding API.

Nyholm commented 8 years ago

Awesome work. Am I correct if I say that nothing has changed in the Baidu API from version 1 to 3 regarding geocoding and reverse geocoding?

vicchi commented 8 years ago

@Nyholm version 1 and version 3 of what? Sorry, don't follow you

But to clarify. This PR is a manual merge of #33 with no changes to the geocoding source code plus the fixes needed to get Baidu's unit tests to build clean.

If that helps!

Nyholm commented 8 years ago

I see that this PR updates the version of Baidu API.

$before = 'http://api.map.baidu.com/geocoder?output=json&key=%s&address=%s';
$after  = 'http://api.map.baidu.com/geocoder/v2/?output=json&pois=0&ak=%s&address=%s';

What have changed on the Baido API server between those releases? Did the change effect the geocode or reverse geocode endpoints?

vicchi commented 8 years ago

@Nyholm Ah. I see what you mean now. The changes to the API endpoints were made in PR #33 and I manually merged that PR into my checked out repo before creating this PR.

The only change on Baidu's end is the URL endpoint, from api.map.baidu.com/geocoder to api.map.baidu.com/geocoder/v2 and also of the inclusion of the pois=0 query string parameter. The payload returned by the Baidu API appears to be consistent between the v1 and v2 endpoints.

The main reason behind PR #48 (this PR), is that the original PR changed the provider's code, but did not change/fix up the unit tests (if indeed they were ever run).

So this PR incorporates and obsoletes PR #33 as well as adding the fixes to get the unit tests to build cleanly again. So when we merge this PR, we'll need to close down #33 as well, but not merge #33.

If that all makes sense?

Nyholm commented 8 years ago

Yes, Everything makes perfect sense. It was this I was looking for:

The only change on Baidu's end is the URL endpoint, from api.map.baidu.com/geocoder to api.map.baidu.com/geocoder/v2 and also of the inclusion of the pois=0 query string parameter. The payload returned by the Baidu API appears to be consistent between the v1 and v2 endpoints.

Nyholm commented 8 years ago

Thank you for your work.

vicchi commented 8 years ago

Awesome. Thanks.