Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #58 #59

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix #58 #59

wants to merge 3 commits into from

Conversation

collinhaines
Copy link

@collinhaines collinhaines commented Jun 9, 2017

This will allow users to use Views Bulk Operations to add subscribed members to an already-created static segment.

A subsequent mailchimp.module patch for adding users will appear on drupal.org.

@Greg-Boggs
Copy link
Contributor

The function already has 5 parameters which is already too many. Is there a way to solve this without adding a 6th?

@collinhaines
Copy link
Author

I attempted to remove (object) entirely but I'm not familiar with MailChimp API enough to know what other submodules require it.

@Greg-Boggs
Copy link
Contributor

Greg-Boggs commented Jun 9, 2017

There's already an options array. Could this feature be one of the keys on the options array?

@collinhaines
Copy link
Author

The problem lies with the object casting. It converts

$array = array(
  'members_to_add' => array(
    '[email protected]'
  )
);

into {"members_to_add":{"0":"[email protected]"}} instead of {"members_to_add":["[email protected]"]} like the example gives in the docs.

@Greg-Boggs
Copy link
Contributor

Greg-Boggs commented Jun 9, 2017

Actually, we'd want the opposite so it doesn't change the existing behavior

if ($options['convert_to_object']) {
          $options['json'] = (object) $parameters;
} else {
          $options['json'] = $parameters;
}

@collinhaines
Copy link
Author

Ok, I believe this is ready to be reviewed. I have another PR ready for the addSegment() function that follows this code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants