Change scheme for the google maps api call to https and include an api key if one was set by matrikular · Pull Request #10439 · joomla/joomla-cms

matrikular · 2016-05-12T14:33:25Z

Summary of Changes

This PR changes the api scheme / protocol for the geocodeAddress call to https instead of http. Currently the JGoogleEmbedMaps::geocodeAddress() api call uses http.

Though, e.g. if you call the api url directly, google redirects you to https, an exception will be returned from google when using the api together with an api key without using https.

object(JHttpResponse)[1247]
public 'code' => int 200
public 'headers' =>
array (size=12)
'Content-Type' => string 'application/json; charset=UTF-8' (length=31)
'Date' => string 'Thu, 12 May 2016 10:18:22 GMT' (length=29)
'Pragma' => string 'no-cache' (length=8)
'Expires' => string 'Fri, 01 Jan 1990 00:00:00 GMT' (length=29)
'Cache-Control' => string 'no-cache, must-revalidate' (length=25)
'Access-Control-Allow-Origin' => string '*' (length=1)
'Server' => string 'mafe' (length=4)
'X-XSS-Protection' => string '1; mode=block' (length=13)
'X-Frame-Options' => string 'SAMEORIGIN' (length=10)
'Accept-Ranges' => string 'none' (length=4)
'Vary' => string 'Accept-Language,Accept-Encoding' (length=31)
'Transfer-Encoding' => string 'chunked' (length=7)
public 'body' => string '{
"error_message" : "Requests to this API must be over SSL. Load the API with "https://" instead of "http://".",
"results" : [],
"status" : "REQUEST_DENIED"
}
' (length=172)

Speaking of the api key; The google library allows you to set an api key that will be used in the javascript rendering part but not in the geocodeAddress() method. This PR also adds a key to the request if present so that you won't run into your quota limit any time soon.

Testing Instructions

Download and install the test modul I attached to this PR. Enable the module, assign it to e.g. the position-3 of the protostar template. Don't forget to set the module to show on all pages.

Depending on the settings (key, address) you've made, you will see a debug message with information about the last request. Now, there aren't really that much visible errors to indicate something not working correctly. The debug message in the module only show that the request works as before.

You can provoke the exception mentioned above by applying the patch and changing the https back to http on line 785 in /libraries/joomla/google/embed/maps.php. Given you've provided an api key you would see a message similar to this one:

Attachment

mod_geocodepr.zip

While the main change to this file is about setting the scheme to https, I've also refactored the uri handling and added the option to apply an api key as well.

…in toString()

…ors with e.g. isSercure()

@robert

Remove the sensor parameter since it doesn't seem to be need any longer. Thanks for pointing it out @robert.

brianteeman · 2016-05-12T16:54:43Z

@mbabker is this a PR that should be made to the framework as well / instead ?

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10439.}

…rely on it Add the sensor parameter back to the api url since the tests seem to rely on it. If that really is the case, we should implement be a better way of "parsing" the url and / or looking for an array index in the tests.

This time the test(s) shouldn't fail - fingers crossed.

kolvar · 2016-05-13T07:02:53Z

Works as described. Fixes a problem I have had. Thanx for the patch.

bembelimen · 2016-05-13T07:59:41Z

I have tested this item ✅ successfully on fd2305c

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10439.}

brianteeman · 2016-05-13T08:01:36Z

MERGER - I am setting this RTC but not sure if should be merged here or in the upstream package?

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10439.}

mbabker · 2016-05-13T11:38:52Z

Yes it needs to be merged here because the CMS isn't using any of the HTTP adapter code (which includes all of the API packages) from the Framework.

brianteeman · 2016-05-13T12:24:43Z

Thanks for confirming @mbabker

andrepereiradasilva · 2016-05-13T14:25:25Z

IMHO it would be a good practice to make this change also in the joomla framework repository
https://github.com/joomla-framework/google-api/blob/master/src/Embed/Maps.php#L685

mbabker · 2016-05-13T14:26:29Z

Should be fixed there, yes. But since it's not a Composer pulled library here, it should be merged here and dealt with on the FW separately.

andrepereiradasilva · 2016-05-13T14:28:29Z

@mbabker yes of course. This is RTC and can be merged.
The framework PR is another thing.

@matrikular can you do a PR also in the framework repository?
https://github.com/joomla-framework/google-api/blob/master/src/Embed/Maps.php#L685

matrikular · 2016-05-13T17:48:17Z

Not sure if it has got the correct format, but - of course - here you go: #10.

zero-24 · 2016-05-16T20:37:55Z

@joomla-cms-bot. This here is one more to fix :P

Change api scheme to https

2ab7599

While the main change to this file is about setting the scheme to https, I've also refactored the uri handling and added the option to apply an api key as well.

joomla-cms-bot added the PR-staging label May 12, 2016

Remove urlencode from address since it will be handled automatically …

5447bc6

…in toString()

matrikular changed the title ~~Change api scheme for the maps api call to https and include an api key if one was set~~ Change scheme for the google maps api call to https and include an api key if one was set May 12, 2016

matrikular added 2 commits May 12, 2016 16:55

Create a new JUri instance instead of using $this->uri to prevent err…

eec02ad

…ors with e.g. isSercure()

Remove the sensor parameter since it doesn't seem to be need any longer

d3c9f31

Remove the sensor parameter since it doesn't seem to be need any longer. Thanks for pointing it out @robert.

matrikular added 3 commits May 12, 2016 20:44

Satisfy Travis, maybe?!

ef6b6cd

Fix url parsing to properly handle query params

7e18a4d

joomla-cms-bot added the Unit/System Tests label May 12, 2016

Remove the sensor parameter since it doesn't seem to be needed any more

fd2305c

This time the test(s) shouldn't fail - fingers crossed.

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 13, 2016

brianteeman added this to the Joomla 3.6.0 milestone May 13, 2016

matrikular mentioned this pull request May 13, 2016

Change scheme for the google maps api call to https and include an api key if one was set joomla-framework/google-api#10

Merged

rdeutz merged commit 270a1df into joomla:staging May 16, 2016

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 16, 2016

matrikular deleted the patch-12 branch July 27, 2017 07:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Change scheme for the google maps api call to https and include an api key if one was set#10439

Change scheme for the google maps api call to https and include an api key if one was set#10439
rdeutz merged 8 commits intojoomla:stagingfrom
matrikular:patch-12

matrikular commented May 12, 2016 •

edited

Loading

Uh oh!

brianteeman commented May 12, 2016

Uh oh!

kolvar commented May 13, 2016

Uh oh!

bembelimen commented May 13, 2016

Uh oh!

brianteeman commented May 13, 2016

Uh oh!

mbabker commented May 13, 2016

Uh oh!

brianteeman commented May 13, 2016 via email

Uh oh!

andrepereiradasilva commented May 13, 2016

Uh oh!

mbabker commented May 13, 2016

Uh oh!

andrepereiradasilva commented May 13, 2016 •

edited

Loading

Uh oh!

matrikular commented May 13, 2016

Uh oh!

zero-24 commented May 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Uh oh!

Conversation

matrikular commented May 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Attachment

Uh oh!

brianteeman commented May 12, 2016

Uh oh!

kolvar commented May 13, 2016

Uh oh!

bembelimen commented May 13, 2016

Uh oh!

brianteeman commented May 13, 2016

Uh oh!

mbabker commented May 13, 2016

Uh oh!

brianteeman commented May 13, 2016 via email

Uh oh!

andrepereiradasilva commented May 13, 2016

Uh oh!

mbabker commented May 13, 2016

Uh oh!

andrepereiradasilva commented May 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matrikular commented May 13, 2016

Uh oh!

zero-24 commented May 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

matrikular commented May 12, 2016 •

edited

Loading

andrepereiradasilva commented May 13, 2016 •

edited

Loading