Problem/Motivation
As the access token has an expiration time, storing it as configuration makes it hard/impossible to store the configuration in a VCS and use automatic deployments.
Proposed resolution
Use the state system instead (https://www.drupal.org/developing/api/8/state)
Remaining tasks
(reviews needed, tests to be written or run, documentation to be written, etc.)
User interface changes
None
API changes
None
Data model changes
access_token, expires_at, redirect_uri and refresh_token are removed from google_analytics_reports_api.settings. Instead the are stored using the state system under google_analytics_reports_api.access_token, google_analytics_reports_api.expires_at, etc.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | 2795115-10.patch | 17.74 KB | hussainweb |
| #10 | interdiff-8-10.txt | 4.1 KB | hussainweb |
| #8 | 2795115-8.patch | 16.95 KB | hussainweb |
| #6 | 2795115-6.patch | 16.84 KB | casey |
| #4 | 2795115-2.patch | 9.99 KB | casey |
Comments
Comment #2
casey commentedComment #3
casey commentedComment #4
casey commentedComment #5
zyyz commented@casey
Good idea. I was thinkiing about doing this myself, But wouldn't this change break existing setup? Maye an update hook should also be added to move access token from config to state storage
Comment #6
casey commentedThis patch also includes some moving around of code. There were redirects issued while the process just continued. It seems there are more headers send later in the process. When the webserver is behind a Varnish reverse proxy, this (incorrect headers/order?) results in a 503.
It also includes a update script moving the access token from config to state.
Comment #7
othermachines commentedI tested the module with patch #6 applied both before installation as well as after (to test update of existing config). All looks good!
google_analytics_reports 8.x-3.x-dev
Drupal 8.2.7
Comment #8
hussainwebI think this is rather important. The only workaround right now is to use config_ignore and even if that was not a lot of work, it is not a good idea to use something like tokens in config. I am okay with storing client ID and secret in config (but NOT exporting) similar to other similar modules. But token doesn't make sense.
Just rerolling the patch for now.
Comment #9
hussainwebIMHO, this is a major issue (at least for the token bit as explained in the previous comment). If the maintainers disagree, please set it back appropriately.
Comment #10
hussainwebI am making some small changes for better readability and standards. I think it could be even better but this is what I could do so far.
Comment #11
hussainwebNext: google_analytics_reports.metadata_last_time also should be in state.
Comment #12
rajab natshahComment #14
rajab natshahComment #15
rajab natshahComment #16
rajab natshahComment #17
rajab natshahCommitted ... Thank you :)
Comment #18
rajab natshah#3102925: Add an automated testing for the [Google Analytics Reports] and API module