Hello,

I have a MailChimp custom text field called Site Roles and we sync to it our Drupal "Site Roles", which has always worked until we upgraded from the 7.x-2.x branch to 7.x-3.2.

In the past, a CSV list of roles was send to MailChimp. Now, nothing is sent (or maybe nothing is received) and every record in MailChimp that is new or has been updated since the upgrade is now blank.

I'm submitting this as a bug report because it's something that used to work just fine in previous versions. I've also marked it as Major priority which I hope is OK because it is actually Critical to us! We send messages out to our list largely depending on their site role. Many people on our list have 2 or more roles, but even those that are just authenticated users have blank entry.

Thanks in advance for any help and pointing us int eh right direction to fix this.

Steve

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

palagpatski’s picture

Hello, any updates on this one? Anybody else noticed this problem? This would be a huge problem for sites that send out emails based on their users' roles.

Actually, it is the 3.x-dev version that's not syncing the roles, just to be clear. However, the stable 3.2 version still has problem with it in that it syncs not the human readable value, but a numeric equivalent or something.

I can probably make some changes to the code and provide a patch, but I need help in finding where that bit of code is found. So if anyone can give me a clue, I would greatly appreciate it.

Thanks in advance!

palagpatski’s picture

Oh, I made a mistake. I accidentally set the user's account creation date as the merge field for Site roles, that's why it's showing the numeric values in the Mailchimp list. Sorry about that. I have corrected it now, Merge field for Site roles is now set to User roles.

But here's the thing, it's REALLY NOT SYNCING the roles. It now outputs a blank field in the MC list.

palagpatski’s picture

Also FYI, I have tested this functionality (syncing users roles) on version 2.x and it just works. So for version 3.x, this functionality is just lost.

ruscoe’s picture

Assigned: Unassigned » ruscoe
palagpatski’s picture

Hi ruscoe, thanks for looking into this. Let me know how I can help.

Anonymous’s picture

Assigned: ruscoe »

Hey Dan, I can take a look at this one.

Anonymous’s picture

Version: 7.x-3.2 » 7.x-3.x-dev
junaidpv’s picture

Status: Active » Needs review
FileSize
1.38 KB

I debugged the issue and found that mailchimp_lists module is preparing array role ids (integers) for user role field. But, that array of integers are not accepted by mailchimp list subscription processing function. I looked on 2.x version of the module and found the it was using token replace function to get single string for a field.

So, I prepared a patch that will call token_generate() to get value for field if it is array of values. He is that patch attached. The work has been sponsored by Wickwood.

wickwood’s picture

Thanks again, Junaid!

eojthebrave’s picture

I don't think the solution in #8 is going to work here. For a few reasons. First off, that check for instanceof EntityListWrapper just above where this patch applies is never going to evaluate to TRUE, it's checking the $wrapper object which is always going to be a standard wrapper. Instead, we should check $wrapper->{$element} for it's type.

Second, in this particular example, I don't think there is a token for [user:roles] - see user_tokens(). So this isn't going to work for replacing the roles.

I tested this locally, and couldn't get it to work. :/

So then I played around a bit and came up with another solution that leverages the EntityMetadataWrapper class a bit more. It seems to be working in my testing for entity properties, fields attached to an entity, term reference fields attached to an entity, and for multi-value fields and properties. It coerces a multi-value field into a comma separated list of field values.

I tested these scenarios.

MERGEVAR = user property like email address
MERGEVAR = user property like roles (multi value)
MERGEVAR = single value text field attached to user entity
MERGEVAR = single value term referemcne field attached to user entity
MERGEVAR = multi value term referemcne field attached to user entity

janaidpv, and wickwood, does the patch in #8 work for you with MailChimp 7.x-3.x? I couldn't get it working at all. :/

Anyway, here's my attempt at getting this working.

FWIW, without this patch using things like a term_reference field for MERGEVAR assignment is also broken. So you can't use a taxonomy term (multi, or single value) in a MERGEVAR.

eojthebrave’s picture

Also, at a minimum. In order for any multi-value field or property to work this line in _mailchimp_lists_mergevars_populate() needs to be changed from:

$object = $wrapper;

to:

$object = $wrapper->{$element};

This is required to maintain the ability to retrieve the first listed value of a multiple value field. Though, even with that we'll need to add some extra handling for reference type fields like term reference or entity reference since $object[0]->value(); will return the Taxonomy object, not the term name. For the term name you probably want $object[0]->label(); .

Maybe one thing we need to decide is do we want to support multiple value fields and properties by concatenating their values, or do we just want to only ever deal with a single value? If we do want to concat strings then the previous patch is probably a good start. This issue #2477191: _mailchimp_lists_subscription_has_changed triggers exception from EntityMetadataWrapper when a value is added to an empty field is also likely to be affected by the same issue with supporting multiple value fields.

wickwood’s picture

Thanks for your interest in this eojthebrave!

I have't had time yet to evaluate your alternate solution, but I did want to let you know that we the solution JunaidPV provided us did work and that for our use case we need a single field that concatenates all of the roles into a single mergefield on MailChimp. This allows us to segment the list with a simple search on MailChimp so we can send to different users with a given role on the Drupal site.

I would think that this would probably be the use case for any multivalued field that needs to be synced with MailChimp.

I don't really understand your alternative suggestion of "or do we just want to only ever deal with a single value?" would work for us, because we need to be able to have all the values on MailChimp. I'm probably missing something in terms of what you mean.

This use to work just fine in the previous versions of this module, but it broke with 7.x-3.x.

I'll test your patch in our environment this weekend or sooner if possible.

eojthebrave’s picture

I don't really understand your alternative suggestion of "or do we just want to only ever deal with a single value?" would work for us, because we need to be able to have all the values on MailChimp. I'm probably missing something in terms of what you mean.

The way the code is currently written, it looks like if you have a field attached to your entity with multiple values that it will only ever send the fist value in the list to MailChimp. Specifically this line $object = $object[0];. This to me reads as, "If there is more than one value take the first one in the list and ignore the rest".

It's different for the roles data though, which is kind of unique in this case. In Entity API terms, the roles on the $user object are a property, and not a field. When the Entity API system coerces the array of roles into an object it turns it into an EntityDrupalWrapper object, whereas a multivalue field becomes and EntityListWrapper object. I'm guessing that the code in place works for the user roles because EntityDrupalWrapper::value() returns just a dumb array, and then the patch in #8 exploits that. But, EntityListWrapper::value() will an array of EntityDrupalWrapper objects. Hence my version being quite a bit more complicated. :/

Though, I can't really test it since I can't even get PHP to enter into the if statement that is supposed to deal with multiple values with the current code. No matter what I do the if ($object instanceof EntityListWrapper) { check never passes.

Out of curiosity, to see if I can get a better understanding of what is going on here, what version of PHP are you using? I've got 5.4.10 running at the moment.

wickwood’s picture

We are using PHP Version 5.3.29.
Hope that helps.

eojthebrave’s picture

I just took another look at the patch in #2477191: _mailchimp_lists_subscription_has_changed triggers exception from EntityMetadataWrapper when a value is added to an empty field, which adds a new helper function for retrieving the value of a $property from an entity. It seems we could do the same thing here, and solve that issue at the same time.

eojthebrave’s picture

Okay, here's a version of the patch that adds a new helper function to extract the value of any property or field from an entity. Then uses that function in both _mailchimp_lists_mergevars_populate() where it's used to set the value of a merge field, and in _mailchimp_lists_subscription_has_changed() where it's used to detect if the value of a field has changed.

This will solve both this issue, and #2477191: _mailchimp_lists_subscription_has_changed triggers exception from EntityMetadataWrapper when a value is added to an empty field which is really similar to this one.

eojthebrave’s picture

Oops. Forgot the patch. Here we go.

eojthebrave’s picture

Bah. That one had an error in it. Variables set to FALSE will still return TRUE from isset(). Classic. :P

eojthebrave’s picture

FileSize
9.48 KB

Okay, here's another approach to this that I think is much simpler. Rather than trying to account for every possible combination of property vs. array property vs. field, etc .. this approach uses the D7 token system. It replaces the field settings form's select options with textfields where you can enter in a token like [user:mail] or [node:author:mail]. This provides for a huge amount of flexibility. And better handling of multi-value fields.

To solve the issue in this thread you can do [user:roles:join:,] to get a comma separated list of user roles.

This also opens up the ability to support non-text type fields in MailChimp. Like for example the Birthday merge field. With the entity_token module enabled (part of the entity api module) you can do the following to coerce any date field in the MM/DD format the MailChimp expects for a birthday. Example: [user:field-birthday:custom:m]/[user:field-birthday:custom:d]. Or convert any date field to a MailChimp date type, [user:created:custom:r].

It also opens up the possibility to do things like [site:name] or any other global token.

With the token module installed you'll get a nice UI for browsing available tokens.

The attached patch also includes an update hook to convert any existing merge field mappings to the new token syntax.

Finally, this will also still resolve #2477191: _mailchimp_lists_subscription_has_changed triggers exception from EntityMetadataWrapper when a value is added to an empty field, and additionally resolves #2008898: Replace Merge fields select options with standard Drupal tokens as well.

jami’s picture

I'm working on getting the patch in comment 18 added in the near future -- thanks for all the great code!

I love the possibility of closing three issues with one patch, but we are wondering if switching all the way to tokens might be confusing for existing users. I like the idea of allowing site managers to use tokens if they prefer that, so I'll leave those relevant bugs open and use your patch (which works great for switching to tokens) when we figure out how to show both the existing dropdown menus and tokens, depending on site manager preference.

Thanks again, and I'll get patch 18 added after I get one more set of eyes on it.

eojthebrave’s picture

If you want to stick with the existing drop-down style for the UX we could probably do so by having the select element values set to their token equivalent. Then maybe provide an "Advanced" toggle, that if someone chooses that option can change the select elements to text fields where they can just add in any token?

Though, the downside of that approach is figuring out what do you put into the select list? Because with tokens we can support a much larger set of content replacement values than we are currently. Hmm.

Actually, the more I think about it ... tokens are already a pretty common Drupal 7 site building pattern. You see it with all kinds of modules, views, path auto, etc. Modules that I'm guessing most people who will be using the MailChimp module are already familiar with. So I think we can feel pretty confident that they'll be able to understand the token UX pattern.

Is there some additional help text that we can add to make this easier for people to understand? If you've got the token module installed the UX gets even better, but it's optional at the moment. We could make it a dependency so that people always get the UX provided by token module for browsing and selecting a token perhaps?

Either way, let me know what if anything I can do to help keep this moving forward. Thanks!

eojthebrave’s picture

Also, FWIW. In case it wasn't clear. The last patch here, which switches to using token module syntax instead of trying to fix the entity API integration, also adds a huge amount of additional functionality that we don't get with any of the other approaches. Things like being able to map a Drupal date field to a MailChimp birthday field, for example, are impossible with the current setup. And using the "non token module" version of this fix is really only a band-aid solution. If we want to support mapping to different types of MailChimp fields other than just "text" in the long run we'll end up with quite a bit of additional code to handle all the various special cases. Using tokens however turns that into more of a documentation/help problem than a technical one.

  • jami committed 0d65de1 on 7.x-3.x
    Issue #2423057 by eojthebrave,junaidpv,wickwood,palagpatski: Syncs multi...
  • db27b47 committed on 7.x-3.x
    Merge pull request #163 from thinkshout/multi-value-field-sync
    
    Issue #...
jami’s picture

Patch 18 is applied, which fixes the very specific original case nicely -- thanks all.

I agree that tokens give us more flexibility, and the "Available tokens" helper is plenty helpful. But I am still hesitant to completely replace the dropdown menu here, and I would prefer to offer both the more flexible token option and the dropdown for familiarity/ease. For now, I will keep tokens in my queue as a feature request until someone has time to make them work side by side with the existing dropdown menus (if that's you, eojthebrave, awesome!). Thanks again.

jami’s picture

Version: 7.x-3.x-dev » 7.x-3.3
Assigned: » Unassigned
Status: Needs review » Closed (fixed)
eojthebrave’s picture

How would you feel about changing the select list to use a token like syntax, and then using the token_* functions to do all of the behind the scenes replacement? This would allow for the select list UI to remain, and for an "Advanced" toggle that would allow you to use a textfield with normal tokens instead.

There are some really big advantages to using token_* to do the value substitution. It eliminates the need for the overly complex Entity API stuff we've got going on currently by eliminating the need for all our edge case handling. And it also helps to resolve a whole bunch of other currently open issues like the module's inability to handle multi-value fields, and the fact that the module has no way of formatting Drupal fields in a MailChimp API appropriate way for things like Birthday and Date fields.

It seems like maybe we should move work for this to #2008898: Replace Merge fields select options with standard Drupal tokens I suppose. Thanks!

wickwood’s picture

By the way, I finally had time to test the new fix and it works perfectly. Thanks to everyone for all your hard work on this! I really appreciate it!