-
Notifications
You must be signed in to change notification settings - Fork 3
added a Settings-Test #1
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
Conversation
With checking a box and press save in the modules config page, a testconnection to MailChimp gets invoked, that pulls the list name, list id and adress settings. If settings are wrong, it reports errors. For example a 401 or 404.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commented some questions, mostly because of my limited knowledge in module dev. Maybe you can have a look before I merge it.
try { | ||
$http = new WireHttp(); | ||
$http->setHeaders([ | ||
//'Content-Type' => 'application/json', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this line commented because it is not necessary? Should I remove it from my get requests too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, your requests have post data that is of type json, so you want to keep it. But I only sent a GET request without other data. Please remove may comment here.
$mc = wire('modules')->get('SubscribeToMailchimp'); | ||
$list = is_null($list) ? $mc->default_list : $list; | ||
$dc = explode('-',$mc->api_key)[1]; // get the datacenter code from the api key, to use it in the api url | ||
$request = "https://{$dc}.api.mailchimp.com/3.0/lists/{$list}/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the getApiBase() method to centralize the Mailchimp API path to faster manage API version upgrades. Can/should we use this here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, at first I used it, but it returns to much. I only need the base, but your method also returns /{$list}/members/
. Maybe you can modify the getApiBase() method with an optional param, so that it returns a base URL, with optionally added string(s) to it:
private function getApiBase($list = null, $param = null) {
$dc = explode('-',$this->api_key)[1];
$return = "https://".$dc.".api.mailchimp.com/3.0/";
if(!is_null($list)) $return .= "lists/".$list."/";
if(!is_null($param)) $return .= $param; // should we automatically add a trailing slash here ?
return $return;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aah, got you. thats great yeah.
// static, because mainly it gets called from static function getModuleConfigInputfields! | ||
// But also can get called from within your template files: $response = SubscribeToMailchimp::testSettings($listID); | ||
public static function testSettings($list = null) { | ||
$mc = wire('modules')->get('SubscribeToMailchimp'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I need this, since the function is already inside the module? Or is it with intent to simulate a normal use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getModuleConfig() is a static method, therefore we need to declare and call the testSettings() method also static. Static methods do not have access to the class (default) params via $this->. But we need $this->api_key, $this->getApiBase($list), or others. Therefore I use $mc as a dynamic(?) class instance. If you have another idea, please tell me or rework my code.
With checking a box and press save in the modules config page, a testconnection to MailChimp gets invoked, that pulls the list name, list id and adress settings. If settings are wrong, it reports errors. For example a 401 or 404.