\Drupal\geolocation_google_maps\Plugin\geolocation\Geocoder\GoogleGeocodingAPI::geocode sets $query_params['key'] manually, based on presence of server api key.

But then calls $this->googleMapsService->getGoogleMapsApiUrl which overwrites the server key with the javascript api key via \Drupal\geolocation_google_maps\GoogleMapsService::getGoogleMapsApiParameters

Issue fork geolocation-3550181

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

aaronbauman created an issue. See original summary.

aaronbauman opened merge request !116

aaronbauman’s picture

aaronbauman
he/him
commented
Status: Active » Needs review

MR 116 adds a test to demonstrate the server key problem, and provide a simple fix.

steven jones’s picture

steven jones commented
Status: Needs review » Needs work

Tests are failing with this fix, but also, I'd be tempted to say that the fix isn't really addressing the general problem, which is that the options are being merged in the wrong order really.

At the moment the code is:

$parameters = NestedArray::mergeDeep($additional_parameters, $custom_parameters, $module_parameters, $geolocation_parameters);

But that means that the base parameters always win, with no possibility to change that.

Seems like they should at least be:

$parameters = NestedArray::mergeDeep($geolocation_parameters, $additional_parameters, $custom_parameters, $module_parameters);

And even then, there would be an argument for doing:

$parameters = NestedArray::mergeDeep($geolocation_parameters, $custom_parameters, $module_parameters, $additional_parameters);

I think?

steven jones opened merge request !117

aaronbauman’s picture

aaronbauman
he/him
commented

I agree, changing the order would make more sense to me.
I didn't want to introduce a breaking change without knowing why they were in this order, but if you are more familiar with how this works then I will defer to you.

steven jones’s picture

steven jones commented

The first time I looked at the code was yesterday! But to my eyes the order does need changing like I've proposed in the MR.

christianadamski made their first commit to this issue’s fork.

christianadamski’s picture

christianadamski commented
Status: Needs work » Fixed

Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

christianadamski closed merge request !116

steven jones’s picture

steven jones commented

Thanks for merging this, don't forget to update the contribution record here: https://new.drupal.org/contribution-record/11426739 ticking the appropriate boxes and pressing save :) thanks!

christianadamski’s picture

christianadamski commented

Didn't d.o do this automatically? Also, this is your MR I merged? I did checkmark thing, tell me if it works.

steven jones’s picture

steven jones commented

It doesn't do it automatically no :(

Thanks for giving me credit, yes, looks like you've ticked the right boxes. You might want to give yourself credit too, for actually doing the commit etc.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.