The Mollom PHP library sets the server endpoint as a public variable. We should expose this through the Mollom settings screen and allow it to be set via the Drupal module.

CommentFileSizeAuthor
#18 2381343.mollom.endpoint.18.patch7.59 KBeshta
#16 2381343.mollom.endpoint.13.patch13.02 KBeshta
#13 2381343.mollom.endpoint.13.patch11.64 KBeshta
#11 2381343.mollom.endpoint.11.patch9.6 KBeshta
#9 2381343.mollom.endpoint.9.patch8.95 KBeshta
#4 2381343.mollom.endpoint.4.patch2.06 KBeshta
#1 2381343.mollom.endpoint.1.patch2.7 KBeshta
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eshta’s picture

eshta CreditAttribution: eshta commented
Status: Active » Needs review
FileSize
2381343.mollom.endpoint.1.patch2.7 KB

Here's the first try.... it is still lacking in test updates, however.

Status: Needs review » Needs work

The last submitted patch, 1: 2381343.mollom.endpoint.1.patch, failed testing.

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented 
  1. +++ b/mollom.admin.inc
@@ -884,6 +884,12 @@ function mollom_admin_settings($form, &$form_state) {
+  $form['access-keys']['mollom_api_endpoint'] = array(
+    '#type' => 'textfield',
+    '#title' => t('Mollom API endpoint'),
+    '#field_prefix' => t('http(s)://'),
+    '#default_value' => variable_get('mollom_api_endpoint', $mollom->server),
+  );

    I'm not sure if it has to be an UI option. A variable that one can set is more than enough imho. If we make it a UI option, it rapidly becomes easy for someone to mess with this. Unless we expose our customers to some website where they can choose different endpoints based on their region or if they want to be on the beta platform they can choose to be? 

  2. +++ b/mollom.admin.inc
@@ -1072,6 +1078,8 @@ function mollom_admin_settings_prepare_submit($form, &$form_state) {
+  // Strip any protocol from the endpoint url.
+  $form_state['values']['mollom_api_endpoint'] = preg_replace('/^(http:\/\/)|^(https:\/\/)/', '', $form_state['values']['mollom_api_endpoint']);

    I think there is a better function for this. Use the https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_... function and then recompile it without the http or whatever prefix they give it. If they set it to ftp, it should also be removed :)

eshta’s picture

eshta CreditAttribution: eshta commented
FileSize
2381343.mollom.endpoint.4.patch2.06 KB

Thanks @Nick_vh. Unfortunately drupal_parse_url just includes the scheme in the path return if it is an external URL so it won't help us here. For stripping of protocols, a full list would be pretty long, but I could switch this to a regex like /^[a-z][A-Z]*:\/\// (need to test that rather than writing off the top of my head).

Of course this is moot if we're not going to expose it in the form, lol. I'm fine with leaving this out for now and adding it in if we find we have a larger-scale need to allow site administrators to specify their endpoint.

There are a bunch of test failures in the installation test that I'm seeing in the dev branch as well as this patch. I'm uploading the patch with failures now, but I'm going to figure out what's going on with these before any commits.

eshta’s picture

eshta CreditAttribution: eshta commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: 2381343.mollom.endpoint.4.patch, failed testing.

Status: Needs work » Needs review

eshta queued 4: 2381343.mollom.endpoint.4.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 4: 2381343.mollom.endpoint.4.patch, failed testing.

eshta’s picture

eshta CreditAttribution: eshta commented
Status: Needs work » Needs review
FileSize
2381343.mollom.endpoint.9.patch8.95 KB

Running this to see what the testbots say.....I think there are some local differences in my environment.

Status: Needs review » Needs work

The last submitted patch, 9: 2381343.mollom.endpoint.9.patch, failed testing.

eshta’s picture

eshta CreditAttribution: eshta commented
Status: Needs work » Needs review
FileSize
2381343.mollom.endpoint.11.patch9.6 KB

Minor tweak.

Status: Needs review » Needs work

The last submitted patch, 11: 2381343.mollom.endpoint.11.patch, failed testing.

eshta’s picture

eshta CreditAttribution: eshta commented
FileSize
2381343.mollom.endpoint.13.patch11.64 KB

OK - so here is a full and final patch with tests in place. The patch will fail the testbots here, but that is due to an unrelated testing issue that has its own issue here: https://www.drupal.org/node/2391671

eshta’s picture

eshta CreditAttribution: eshta commented
Status: Needs work » Needs review

Just to make sure the rest of the tests are still alright ;-)

Status: Needs review » Needs work

The last submitted patch, 13: 2381343.mollom.endpoint.13.patch, failed testing.

eshta’s picture

eshta CreditAttribution: eshta commented
Status: Needs work » Needs review
FileSize
2381343.mollom.endpoint.13.patch13.02 KB

Fixing the unexpected test failures...

Status: Needs review » Needs work

The last submitted patch, 16: 2381343.mollom.endpoint.13.patch, failed testing.

eshta’s picture

eshta CreditAttribution: eshta commented
Status: Needs work » Needs review
FileSize
2381343.mollom.endpoint.18.patch7.59 KB

Reroll

  • eshta committed b2bf3e8 on 7.x-2.x 
    Issue #2381343 by eshta: Allow Mollom API endpoint to be configurable.
eshta’s picture

eshta CreditAttribution: eshta commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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