Mailchimp are currently several hours into a geographic outage. An opportunity to make the module more robust!
* In mailchimp_get_api_object() the client timeout was hardcoded to 60 seconds, despite being configurable in the bundled Mailchimp client library.
* The bundled client library has deprecated $timeout parameter to new client instantiation, replacing it with $http_options which supports $http_options['timeout'].
* mailchimp_campaign_list_templates() does not catch exceptions as it is done through much of the rest of the module.
* mailchimp_campaign_entity_storage_load() does not check if $mc_campaigns[$mc_campaign_id] exists before using it.
* document the timeout setting
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | mailchimp-more_robust_for_outages-2998712-16.patch | 1.57 KB | wxactly |
| #13 | interdiff_12_13.txt | 548 bytes | gold |
| #13 | mailchimp-more_robust_for_outages-2998712-13.patch | 3.27 KB | gold |
Comments
Comment #2
xurizaemonComment #3
xurizaemonPS. If you'd like any of this split into separate issues or patches, just say so - happy to oblige. With 280 open issues, I figured it would be simpler to submit it as a single bite :)
Comment #4
xurizaemonOn review, I'm not sure that timeout setting is a hidden setting - pretty sure I saw it referenced somewhere in config, but maybe that was MC API config not Mailchimp module config. This PR treats it as one, in which case we perhaps should add it to config/install/mailchimp.settings.yml?
If not existing config, I propose we add it. Hardcoded 60 second timeout was a sitekiller. MC API seems to default to 10s.
Comment #5
thursday_bw commentedxurimon.. :) You have copied your commit information from your console in this patch along with your diff, so.. it's not going to apply.
I think it would be worth breaking the timeout issues and the mailchimp_campaign_entity_storage_load() into separate issues.
Comment #6
xurizaemonThanks.
That patch seems to apply OK from this project's composer.json? I think it's fine, that's the patch format generated when I
git showto generate a patch. The maintainer can optionally use that or apply their own commit message. See this SE question for more.Agree on mailchimp_campaign_entity_storage_load.
Comment #7
zerolab commentedIt is worth replacing the use of
drupal_set_message()which got deprecated in Drupal 8.5.0 and replace with\Drupal\Core\Messenger\MessengerInterface::addMessage()Comment #8
xurizaemon@zerolab, while it's a trivial change (patch), I feel like that should be a separate issue, since:
drupal_add_message()in this patch would leave several other usages in the same file unmatched>= 8 < 8.5, but that change would break sites below 8.5 wouldn't it? (I guess they can just use an older Mailchimp module?)Since this is all procedural code here, the patch linked uses the static
\Drupal::messenger()->addMessage('foo');syntax.Comment #9
xurizaemonI've opened separate issue #3019881: Update relevant services to use dependency injection for @zerolab's proposed change in comment 7.
Comment #10
goldA couple of things broke this patch and some other issues made other bits redundant.
Rerolled the patch. Still needs review.
Comment #11
thursday_bw commentedLooks good to me, reviewed and tested.
I do say I don't like the number of hooks being used. Worth some investigation as to whether some can be replaced by events. But that's
a separate issue altogether.. would be great to get this patch through, it's clean, simple, makes sense and works.
Cleaning this issue of the list makes space for energy to be spent on more important features.
Comment #12
brendanthinkshout commentedWhile the change in #3081006 did update the HTTP options structure in mailchimp.module, it didn't incorporate the ability to retrieve a timeout length from config, which is part of the motivation for this issue. Rerolling to incorporate that line.
Comment #13
goldGood catch. :) However, Config::get() doesn't take a default value like that.
Checking how to do this correctly it appears
?:is the right approach. There's a good summary of this on issue #2090751, which was looking at adding this functionality, but ultimately closed as "Closed (won't fix)."Comment #14
brendanthinkshout commentedI suspect that is the right approach, but the above patch no longer applies to the most recent commit on 1.x. Will reroll.
Comment #15
gcbComment #16
wxactly commentedA configurable timeout value is being set with the fix for https://www.drupal.org/project/mailchimp/issues/2768455
I re-rolled this patch so that it only has the additional robustness for the campaign template list call.
Comment #18
wxactly commentedComment #20
gcb