Problem/Motivation

When the module was not able to retrieve any subscription lists, it shows the message

The subscription service is currently unavailable. Please try again later.

to the user.

Besides, that that message could be misleading, because it's quite generic, the code execution in the buildForm() method continues and builds a subscription form with an undefined $list->id in its wrapper. Through the mergevars processing afterwards all form elements will be disabled, to there is no further business logic affected.

The notice thrown is

Notice: Trying to get property of non-object in Drupal\mailchimp_signup\Form\MailchimpSignupPageForm->buildForm() (line 119 of /modules/contrib/mailchimp/modules/mailchimp_signup/src/Form/MailchimpSignupPageForm.php).

Proposed resolution

Do a proper isset check, before adding the $list-id to the wrapper element, to avoid the notices.

CommentFileSizeAuthor
#6 mailchimp-undefined_mergevar_form_wrapper-2776765-6.patch799 bytesszeidler
#2 mailchimp-undefined_mergevar_form_wrapper-2776765-2.patch644 bytesszeidler

Comments

szeidler created an issue. See original summary.

szeidler’s picture

szeidler commented
Title: Hide Signup form, when not list available » Check for undefined mergevar list-id, when no list available
Status: Active » Needs review
StatusFileSize
new mailchimp-undefined_mergevar_form_wrapper-2776765-2.patch644 bytes

Attached patch for a conditional list-id wrapper. I'm open for other fixes here, the important thing is to get rid of the notice and get the logs clean for the important entries.

  • amyvs committed 9be504e on 8.x-1.x authored by szeidler 
    Issue #2776765 by szeidler: Check for undefined mergevar list-id, when...
amytswan’s picture

amytswan commented
Status: Needs review » Fixed

Thanks for finding this issue and proposing a patch, @szeidler! This patch looked like a good way to handle this issue, so I committed it.

idebr’s picture

idebr commented
Status: Fixed » Needs work 
+++ b/modules/mailchimp_signup/src/Form/MailchimpSignupPageForm.php
@@ -111,6 +111,7 @@ class MailchimpSignupPageForm extends FormBase {
+    $mergevars_wrapper_id = isset($list->id) ? $list->id : '';
...
       '#prefix' => '<div id="mailchimp-newsletter-' . $list->id . '-mergefields" class="mailchimp-newsletter-mergefields">',

This change does not actually solve the problem, since the new variable is not applied to the template.

szeidler’s picture

szeidler commented
Status: Needs work » Needs review
StatusFileSize
new mailchimp-undefined_mergevar_form_wrapper-2776765-6.patch799 bytes

Ouch, you're absolutely right. Attached an additional patch to fix the issue, based on the current dev, which has the "broken" patch already included.

asrob’s picture

asrob
he/him
Primary language Hungarian
Location Hungary 🇭🇺 🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

I've successfully applied this patch and looks good to me. RTBC'ed!

gnuget’s picture

gnuget
Primary language Spanish
commented

This was committed 3 months ago... wondering why it is still as RTBC?

szeidler’s picture

szeidler commented

The initial patch, which was committed, didn't solve the problem correctly. Therefore the issue was reopened again and an additional patch in #6 went rtbc.

  • amyvs committed 7058b7b on 8.x-1.x authored by szeidler 
    Issue #2776765 by szeidler: Check for undefined mergevar list-id, when...
greg boggs’s picture

greg boggs
he/him
Primary language English
Location Portland Oregon
commented

Thanks for all the work on this one folks.

  • Greg Boggs committed d610d60 on 8.x-1.x authored by szeidler 
    Issue #2776765 by szeidler, amyvs, asrob: Check for undefined mergevar...
greg boggs’s picture

greg boggs
he/him
Primary language English
Location Portland Oregon
commented
Status: Reviewed & tested by the community » Fixed
grantkruger’s picture

grantkruger
Location Portland
commented

Due to us having a mailchimp signup form in our site footer users to our site get this message on every page even though they probably are not signing up. They get the message repeatedly on multiple pages/refreshes even if they x it out several times. This happens for some minutes after the caches get flushed. Any news on when these changes are getting released to a new recommended version or do I need to go with the dev version or the patches to fix the problem?

Status: Fixed » Closed (fixed)

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