Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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 CreditAttribution: casey at SWIS commentedComment #3
casey CreditAttribution: casey at SWIS commentedComment #4
casey CreditAttribution: casey at SWIS commentedComment #5
zyyz CreditAttribution: 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 CreditAttribution: casey at SWIS 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 CreditAttribution: 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 Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedComment #14
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedComment #15
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedComment #16
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedComment #17
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedCommitted ... Thank you :)
Comment #18
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commented#3102925: Add an automated testing for the [Google Analytics Reports] and API module