Problem/Motivation
I work on a site which requires us to use ArcGIS and store the results. Per their terms of service, this requires an account with their service in order to provide a token. In order to support this, I have submitted a PR to the Geocoder PHP project (https://github.com/geocoder-php/Geocoder/pull/1090) to update the ArcGISOnline provider class/package.
Proposed resolution
To support this in Drupal I have updated the schema and created a ConfigurableProviderUsingHandlerWithAdapterBase
Provider Plugin so that the credentials necessary for token creation can be entered in the config form. This also caches the token for the valid period. The attached is a patch to add these to the Geocoder Drupal project once the ArcGISOnline provider library has been updated/released.
Comment | File | Size | Author |
---|---|---|---|
#11 | geocoder_arcgisonline_token_3179963-11.patch | 4.48 KB | itamair |
| |||
#8 | 3279963-08.patch-interdiff.txt | 1.9 KB | ekes |
#8 | 3179963-08-arcgisonline-token.patch | 4.48 KB | ekes |
| |||
geocoder--schema-and-handler-for-ArcGISOnline-tokens.patch | 4.52 KB | wsantell | |
|
Comments
Comment #2
wsantell CreditAttribution: wsantell commentedThe PR has been merged and the ArcGIS Provider has been updated to 4.2.0 on both GitHub and Packagist.
Comment #3
itamair CreditAttribution: itamair as a volunteer commentedThanks @wsantell ... great work (you did on this https://github.com/geocoder-php/Geocoder/pull/1090).
Your patch applies cleanly. But there is something that I would make sure & more clear.
This patch is creating a new arcgisonlinetoken geocoder provider, that looks alternative to the existing arcgisonline one.
I didn't inspect the arcgisonline services specifications, so I ask you.
Is your new arcgisonlinetoken adding support to a NEW (and alternative) more pro arcgisonline geocoding service that requires token/clinet_id and client_secret?
Has the previous existing arcgisonline provider still reason to exist/be present?
Comment #4
itamair CreditAttribution: itamair as a volunteer commentedAh ok. I got it. They can both co-exist ...
But I think that you did some confusion in the code.
Why you are mentioning the token and it seems the arcgisonlinetoken provider settings are requesting client_id and client_secret (both required)?
And why here: https://github.com/geocoder-php/Geocoder/blob/master/src/Provider/ArcGIS...
is stating the following:
It seems that you did a little mix (and mess) with code taken from the GoogleMaps provider, from here:
https://github.com/geocoder-php/Geocoder/tree/master/src/Provider/Google...
and from the geocoder_provider.configuration.googlemaps_business geocoder.schema, from here:
https://git.drupalcode.org/project/geocoder/-/blob/8.x-3.x/config/schema...
Definitely it seems to me that you need to interact again with the geocoder-php/Geocoder maintainer to correct this Readm.me file:
https://github.com/geocoder-php/Geocoder/blob/master/src/Provider/ArcGIS...
and Need Work to properly correct this new drupal Geocoder arcgisonlinetoken provider settings,
that is asking both Client ID and Client Secret (as required) instead of a Token value ...
Comment #5
wsantell CreditAttribution: wsantell commented@itamair,
It does look like I need to update this comment on the ArcGISOnline provider readme:
// Client ID is required. Private key is optional.
However, the rest of that instruction is correct for calling
ArcGISList::token
. I did not explain the process for obtaining a token since the link to the instructions is provided in the readme, but to summarize you must provide a Client ID and Client Secret to obtain a token which is only valid for a period of time no longer than 14 days (default is 2 hours).In Drupal, if the config form generated by the
ArcGISOnlineToken
class being added in this patch were used to store the token, the user would need to manually request a new token and enter it on this form every time the token expired. Instead, it works as follows:expires_in
valueArcGISList::token
methodPlease let me know if there are still any points which are unclear or you feel I'm mistaken on.
Comment #6
wsantell CreditAttribution: wsantell commented@itamair,
I have updated the README documentation in my fork:
https://github.com/wantell/Geocoder/tree/arcGISupdate-1089/src/Provider/...
Before I submit this to the main repo, does this documentation update resolve your questions from the perspective of this Drupal project? Do you think it would also help if I modified the
client_id
description, and added one forclient_secret
, to more clearly state the relation between them and the token? For reference this is the description in the initial patch:Thanks in advance for any feedback, and sorry for the slow response. Work is less chaotic now and hopefully I can respond to any requested changes quickly.
Comment #7
wsantell CreditAttribution: wsantell commented@itamair,
Updated documentation has been merged into the Geocoder project:
https://github.com/geocoder-php/Geocoder/pull/1105
Please let me know if you need any other work on this patch.
Comment #8
ekes CreditAttribution: ekes as a volunteer commented@wsantell Thanks for this. I was just wondering how the token handling would work!
My test is against a free account, so I can only get as far as generating a valid token, and using it, to get the error (valid token - but not authorized to store results), which is correct.
During testing it does correctly use the oAuth credentials to get a token. The cached token is checked every time the provider has been loaded. And after expires it was refreshed.
While I was doing this I did a bit of coding standards cleanup:-
There were several services already in the Plugin so we can use the geocoder cache
The plugin has the messenger trait. I've also translated the output, it has the translation trait too. In the example below I just made the message a bit simpler as I couldn't think how to make the previous easily translatable.
There's a Guzzle 6 compatible HttpClient that geocoder provides as a service. I've not looked I guess it's 6 as that's what should be passed on to geocoder-php.
Coding standards like use statements
And this one's looking forward REQUEST_TIME is deprecated.
Last one this is correct for SendRequest as far as I understand it (use statement is Http\Client\Exception\TransferException)
But other than those niggly things that it makes sense here.
Comment #9
ekes CreditAttribution: ekes as a volunteer commentedComment #10
itamair CreditAttribution: itamair as a volunteer commentedOk. Sincerely speaking I don't have now time to test and review the #8 patch @ekes, but I can see your coding standards are quite good, so I am inclined to trust the functionalities of this new arcgistoken geocoder_provider.
But to commit this I need a full RTBC here ...
Comment #11
itamair CreditAttribution: itamair as a volunteer commentedFor now I am adding a new patch that slightly cleans the #8 patch one (better doc for the new ArcGISOnlineToken provider class (and better patch naming complying with drupal patch naming conventions).
Comment #12
itamair CreditAttribution: itamair as a volunteer commented