🇺🇸United States @torgosPizza

I agree that Stripe Customer IDs should be decoupled from payment gateways and considered as a property or field of a Drupal User. For our own integrations in Drupal 7, we wrote custom code that stored the Stripe Customer ID as a field value on the User Entity so that we could correctly process webhooks from Stripe for payment processing (recurring transactions for Subscriptions).

I think if a Drupal User is storing a payment method as a card on file for the Commerce store, it should be stored on the user so that they may reuse the same customer ID for any future purchases; this will help keep Stripe records consolidated and will avoid having multiple Customers listed for Users which could lead to data loss and would require reconciling them later, which can be very challenging and time consuming. So +1 to decoupling Stripe Customer IDs and making them a shared value for Drupal Users.

In my opinion since this is an issue in a -dev version it's not too late to add this into an update hook. It seems pretty important.

Closing as a duplicate of 🐛 Licenses under renewal in progress do not expire Active

This sounds possibly related to 🐛 Licenses under renewal in progress do not expire Active , give that issue a look.

TomTech → credited torgosPizza → .

I posted a comment just now in #3205084: Update to use current Stripe API → because that issue was closed as outdated and pointing to this one instead. But it should be noted that the API Version and the PHP Library are different, and independent of one another. For instance you can update to the latest PHP Library but specify an older API Version in the request. It may take some work to get the module into a position where "any" API version will work, though, so that's a consideration that should be kept in mind.

I don't think this issue should be closed. The problem is that the API Version and PHP Library are two different, independent things. The PHP Library is purely code but the "API" can be selected in the dashboard, and specified in code along with the PHP request. The request and response structures may also change between PHP versions, some of which are not backwards-comptible.

I suggest re-opening this Issue and instead re-titling it to something like "Make module compatible with multiple API versions" which may require some refactoring of the code, so that requests and response considerations can be mapped to the relevant version to avoid exceptions or other API errors.

A setting for include paths would be cool. Keep in mind that a site would also need to add the script to any terminal pages for purposes such as manually entering a payment.

This seems related and relevant: 🐛 The JWT must contain the kid Active

Sorry, I updated the summary. Undoing that.

I found a matching library issue https://github.com/bshaffer/oauth2-server-php/issues/363 and PR: https://github.com/bshaffer/oauth2-server-php/pull/932 which looks like it could resolve it in the Library itself, and then the D7 module (which we are still on, sadly) may need to be updated as well because if we remove a key during rotation then it will still cause tokens to fail decryption.

We are currently experiencing this on our live production site (Drupal 7) and based on what I see in the D8 code, the latest version of the module may suffer from a similar problem.

Right now, applying patches adding classes such as JwtAccessTokenStorage are helpful, but the issue lies in the fact that the private and public keys are both "global". Most of the time this works especially if users are not hitting these frequently.

But, we have seen one particular scenario play out quite frequently (using JWTs, access token expiry = 3600, refresh token = 90 days):

1. A user generates a JWT right before new server keys are needed. (Example: if server keys get generated every day at 23:59:59, our test user should generate their token at 22:59:59.)
2. Server cron runs and new keys are generated.
3. The user continues to make requests to endpoints with the JWT from Step 1 because it is valid for 2 more hours.

Expected result: the access token (which is not yet expired) is accepted as valid.

Actual result: "invalid_token" error because the global keys no longer decrypt the token.

In the D7 module this is due to 1) the keys being global and 2) not being stored with the token. Even when we have configured a Public Key in the settings area, this key is ignored in the encode/decode process. It does appear the latest code follows a similar pattern:

  public function getPublicKey($client_id = NULL) {
    // The library allows for per-client keys. The module uses global keys that
    // are regenerated every day, following Google's example.
    $keys = Utility::getKeys();
    return $keys['public_key'];
  }

I'm not sure how to properly address this, but part of the issue IMO seems to be to ensure that the public key used to encode the JWT is stored in the token itself on the server. If that is not possible, it should be changed so that the public key is NOT regenerated so frequently; while the code block's comments above indicate we are "following Google's example," what the Drupal module neglected to copy was low frequency of public key rotation:

Since Google changes its public keys only infrequently, you can cache them using the cache directives of the HTTP response ...

In the Drupal side of things, private and public keys are rotated every day during oauth2_server_cron(), resulting in frequent access tokens being erroneously seen as "invalid". One other solution might be to make the access tokens only expire after 30 minutes, but we sell video content that is sometimes 2 hours in length, and it's possible that this could cause errors while the user is viewing their purchased content. In my opinion the keys system should be made more flexible to account for this behavior.

The issue appears to be the function _votingapi_bayesian_regenerate() in votingapi_bayesian.helpers.inc. That function performs a recalculation of every vote cast on every entity and stores the results in the cache; however, this is a duplication of effort because the core Voting API module already does this in hook_votingapi_results(). I can write a quick patch that just removes this function so that myself and others can use it.

I could be mistaken, but I believe the different "methods" comes from the fact that when this module was written, only card sources were allowed. Then came sources API, and the Paymentintents API, and the maintainers felt that it was best to keep them separate rather than migrate from Cards to Payments.

I would suggest, rather than overriding classes, writing patches for the module to allow for a cleaner migration path into using just Payments API, which would help others using this module as well as the maintainers. Perhaps that can be a target for the 2.x version of the module.

This worked for us too. Many thanks for the patch!

Linking to parent issue

torgosPizza → created an issue.

Setting to Needs Review since there's a patch here. It also looks like the patch can still apply on 5.x but probably creating a new patch / issue would work better for that.

I'm running into this as well, and it looks like there are actually a few issues. Namely, the Drupal 7 version of the Mailchimp module is no longer in sync with the recent API changes in the Mailchimp 3.x library.

This issue in particular can be fixed by adding 'src/MailchimpApiInterface.php', to the hook_libraries_info() implementation. That will clear up the error in question.

Unfortunately there are quite a few more changes that will be need to made in order to bring this module in line with the Drupal 8+ versions. I'm hopeful that the maintainers will recognize that the Drupal 7 version is still in use on live sites, and with ours we really need the ability to tag Audience members / accounts for promotional marketing emails, so having the module updated would be a huge help. I'll try to create a new meta Issue with some of the tasks that I'm finding as I dig in to make the module compatible with the latest MC API Library.

I think this might have been fixed with the patch in #2919274: Saving a coupon can be extremely slow → .

Got it, thanks @japerry - this is a good point.

Attached is a new patch that creates a new checkbox in the admin form and only includes the shim if it's been enabled.

It also updates the shim JS file to use the window value rather than a pure function block as described in #114.

Please test. Thanks!

@jenlampton: Thanks for the insight, I hadn't tested with multiple IDs but can confirm the same issue you're seeing. I wrote a fix to the patch that I'll upload, it basically loops through each $id_list value and checks for non "UA-" values, adding the shim only once at that point.

However I'm still getting an infinite loop, the problem is still in the "create" call. ga.create() calls gtag.config() which seems to call ga.create() again ad infinitum.

We might just not need the shim at all, since Google seems intelligent enough to include the ga() functions if the property being configured needs it. I commented out the shim and tested with both a G- and a UA- property and it seems to work as expected without requiring the shim. When a "UA-" property ID is present, analytics.js pulls in its own version of ga(). In that case, the function ga() is still defined.

So in my opinion the best approach will be to simply omit the shim altogether. If the project owners are okay with this I can spin up a patch that removes the file and the line of code that loads it. Unless there is a reason that we should include it that I haven't thought of, but anyone using this module will fall under 1 of 2 scenarios:

1) Only using GA4 properties in which case they should never invoke ga() directly;
2) Migrating from UA to GA4, in which case ga() is already defined automatically.

A shim is a backwards-compatible file that usually includes the old functions that otherwise would become unavailable after an upgrade.

The shim included by the patch in this issue provides essentially a translation for the new GA4 function gtag() that accepts calls to the previous function, ga().

So in other words, if you upgrade from the version of Google Analytics that uses gtag(), but some javascript in your site still makes calls to the previous function ga(), you won't run into errors because the shim has provided a new method called ga() which allows you to upgrade smoothly.

It happens! :)

But I am also wondering if we should put loading the shim behind a configuration flag

That's not a bad idea, however I think we'd want to be very explicit about the side effect, e.g. "Your pages may become unresponsive in some browsers due to infinite recursion." Hopefully that'd be enough to ward off anyone turning it on willy-nilly, and then only put it behind a variable like google_analytics_include_shim that overrides the default behavior when TRUE - rather than in the configuration form. The reason being: unless they have /admin pages set to not load the GA script, they may not be able to turn it off quickly if they enable the shim and the page hangs. I'd be concerned if someone doesn't know how to set a variable through Drush in that case.

Here's a very simple patch that uses strpos() to detect whether the ID does not start with UA-, and if that is the case, includes the shim in the script. I'm open to a better solution, just not sure on details regarding the different IDs.

I thought it might have something to do with migrating a UA property to GA4. Google recommends that practice, and for us it's a requirement because we're maintaining our historical data. I think we should set this to Needs Work until there is a way to detect this and only serve the shim as necessary as per @japerry in #115.

From what I can tell in my network calls, the analytics.js call is being requested by the file https://www.googletagmanager.com/gtag/js.

It says it is a function that's being defined in analytics.js.. The log output shows up as ƒ (a){J(1);Z.D.apply(Z,[arguments])}

It appears that removing the gtag-shim.sj file resolves it for me. I tested by simply commenting out this line:

@@ -279,7 +284,7 @@ function googleanalytics_page_alter(&$page) {
     // Build tracker code.
     $script = 'window.dataLayer = window.dataLayer || [];';
     $script .= 'function gtag(){dataLayer.push(arguments)};';
-    $script .= PHP_EOL . file_get_contents(__DIR__ . '/googleanalytics.gtag-shim.js') . PHP_EOL;
+//    $script .= PHP_EOL . file_get_contents(__DIR__ . '/googleanalytics.gtag-shim.js') . PHP_EOL;
     $script .= 'gtag("js", new Date());';
     // Developer ID for Drupal provided by Google.
     $script .= 'gtag("set", "developer_id.dMDhkMT", true);';

It requires to clear "all cache" for the change to get picked up with anonymous visitors due to Drupal page caching. Hopefully this can help us narrow down the root cause and a better fix than removing the shim (which I think would likely be helpful to others).

For what it's worth I am also able to reproduce this behavior with the latest updates from -dev. The behavior goes away if I revert back. We don't have Google Tag Manager installed but we do use a few other JS modules like Stripe. The problem becomes apparent when the browser stalls at the TLS handshake phase. Will be digging in to see if I can isolate the cause.