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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

casey created an issue. See original summary.

casey’s picture

FileSize
9.85 KB
casey’s picture

Status: Active » Needs review
casey’s picture

FileSize
9.99 KB
zyyz’s picture

@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

casey’s picture

FileSize
16.84 KB

This 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.

othermachines’s picture

I 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

hussainweb’s picture

FileSize
16.95 KB

I 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.

hussainweb’s picture

Priority: Normal » Major

IMHO, 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.

hussainweb’s picture

FileSize
4.1 KB
17.74 KB

I 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.

hussainweb’s picture

Next: google_analytics_reports.metadata_last_time also should be in state.

Rajab Natshah’s picture

Title: Use state system for storage of access token » Add use state system for storage of access token

Rajab Natshah’s picture

Assigned: Unassigned » Mohammed J. Razem
Rajab Natshah’s picture

Assigned: Mohammed J. Razem » Unassigned
Rajab Natshah’s picture

Status: Needs review » Fixed
Rajab Natshah’s picture

Committed ... Thank you :)

Rajab Natshah’s picture

#3102925: Add an automated testing for the [Google Analytics Reports] and API module

  • Having an optimization round over the code in the module.
  • Adding Drupal-ci automated testing for selected used testing cases
  • Then release an RC or Stable version

Status: Fixed » Closed (fixed)

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