This patch adds exportables support for Mollom form configurations through loose coupling with the CTools export API. If CTools is available, Mollom form configurations can be exported and any defaults can be overridden and reverted. By virtue of providing CTools integration, Mollom also can be used with Features (see screenshots).

If CTools is not present, Mollom falls back gracefully to the current behavior with no advanced storage handling.

Patched against DRUPAL-6--1.

Files: 
CommentFileSizeAuthor
#91 717874-91.mollom.exportables-7.x-2.12.patch20.86 KBAron Novak
#85 717874-84.mollom.exportables-7.x-2.12.patch21.1 KBAron Novak
FAILED: [[SimpleTest]]: [MySQL] 7,582 pass(es), 12 fail(s), and 1 exception(s).
[ View ]
#85 717874-84.mollom.exportables-7.x-2.x.patch21.1 KBAron Novak
FAILED: [[SimpleTest]]: [MySQL] 7,582 pass(es), 12 fail(s), and 1 exception(s).
[ View ]
#83 717874-83.mollom.exportables.patch21.1 KBeshta
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to create dependency directory.
[ View ]
#74 717874-74-exportables-for-mollom.patch21.3 KBnetsensei
PASSED: [[SimpleTest]]: [MySQL] 7,498 pass(es).
[ View ]
#74 interdiff.txt7.12 KBnetsensei
#72 interdiff.txt1 KBnetsensei
#72 717874-72-exportables-for-mollom.patch21.06 KBnetsensei
PASSED: [[SimpleTest]]: [MySQL] 7,498 pass(es).
[ View ]
#69 717874-69-exportables-for-mollom.patch21.25 KBnetsensei
FAILED: [[SimpleTest]]: [MySQL] 7,498 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#69 interdiff.txt457 bytesnetsensei
#67 interdiff.txt968 bytesnetsensei
#67 717874-67-exportables-for-mollom.patch20.81 KBnetsensei
FAILED: [[SimpleTest]]: [MySQL] 7,498 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#64 interdiff.txt422 bytesnetsensei
#64 717874-64-exportables-for-mollom.patch20.57 KBnetsensei
FAILED: [[SimpleTest]]: [MySQL] 7,498 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#62 interdiff.txt499 bytesnetsensei
#62 717874-62-exportables-for-mollom.patch20.58 KBnetsensei
FAILED: [[SimpleTest]]: [MySQL] 7,498 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#57 interdiff.txt4.31 KBnetsensei
#57 717874-57-exportables-for-mollom.patch20.75 KBnetsensei
FAILED: [[SimpleTest]]: [MySQL] 7,498 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#52 interdiff.txt5.45 KBnetsensei
#52 717874-52-exportables-for-mollom.patch18.76 KBnetsensei
FAILED: [[SimpleTest]]: [MySQL] 7,498 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#47 717874-47-exportables_for_mollom.patch13.6 KBmr.york
PASSED: [[SimpleTest]]: [MySQL] 7,463 pass(es).
[ View ]
#42 717874-42-exportables_for_mollom.patch13.09 KBstella
PASSED: [[SimpleTest]]: [MySQL] 6,710 pass(es).
[ View ]
#31 717874-31-exportables-for-mollom_reroll.patch12.53 KBnetsensei
PASSED: [[SimpleTest]]: [MySQL] 4,884 pass(es).
[ View ]
#29 717874-29-exportables-for-mollom_reroll.patch12.08 KBnetsensei
PASSED: [[SimpleTest]]: [MySQL] 4,883 pass(es).
[ View ]
#23 717874-23-exportables-for-mollom_reroll.patch12.52 KBcam8001
PASSED: [[SimpleTest]]: [MySQL] 4,371 pass(es).
[ View ]
#18 717874-18-exportables-for-mollom.patch11.89 KBGarrett Albright
FAILED: [[SimpleTest]]: [MySQL] 4,226 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#16 717874-16-exportables-for-mollom.patch11.99 KBGarrett Albright
FAILED: [[SimpleTest]]: [MySQL] 2,876 pass(es), 3 fail(s), and 0 exception(es).
[ View ]
#15 717874-15-exportables-for-mollom.patch12.14 KBGarrett Albright
FAILED: [[SimpleTest]]: [MySQL] 2,876 pass(es), 3 fail(s), and 0 exception(es).
[ View ]
#14 717874-14-exportables-for-mollom.patch12.85 KBGarrett Albright
FAILED: [[SimpleTest]]: [MySQL] 106 pass(es), 58 fail(s), and 2 exception(es).
[ View ]
#9 mollom-n717874-9.patch802 bytesDamienMcKenna
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch mollom-n717874-9.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#7 mollom-DRUPAL-6--1.export.7.patch12.71 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch mollom-DRUPAL-6--1.export.7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 717874_mollom_ctools_export_2.patch11.58 KByhahn
PASSED: [[SimpleTest]]: [MySQL] 1,289 pass(es).
[ View ]
screenshot4.png8.06 KByhahn
screenshot3.png14.83 KByhahn
screenshot2.png20.05 KByhahn
mollom_ctools_export.patch7.72 KByhahn
FAILED: [[SimpleTest]]: [MySQL] 1,307 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, mollom_ctools_export.patch, failed testing.

sun’s picture

Thanks! That test failure is caused by a different bug unrelated this patch, #716250: Blacklisting test

+++ mollom.admin.inc 18 Feb 2010 06:32:09 -0000
@@ -21,21 +21,46 @@ function mollom_admin_form_list() {
+  if (module_exists('ctools')) {
...
+    if (module_exists('ctools')) {
...
+    $rows[] = array(array('data' => l(t('Add form'), 'admin/settings/mollom/add'), 'colspan' => module_exists('ctools') ? 5 : 4));

Can we do a single

$exportable = module_exists('ctools');

at the beginning?

+++ mollom.admin.inc 18 Feb 2010 06:32:09 -0000
@@ -21,21 +21,46 @@ function mollom_admin_form_list() {
+      // Remove protection.
+      if ($mollom_form['export_type'] & EXPORT_IN_DATABASE) {
+        $row[] = l(t('Unprotect'), 'admin/settings/mollom/unprotect/' . $form_id);
+      }
+      else {
+        $row[] = '';
+      }
+      // Storage operations.
+      if ($mollom_form['export_type'] & EXPORT_IN_DATABASE && $mollom_form['export_type'] & EXPORT_IN_CODE) {
+        $row[] = t('Overridden (!revert)', array('!revert' => l(t('revert'), 'admin/settings/mollom/revert/' . $form_id)));
+      }
+      else if ($mollom_form['export_type'] & EXPORT_IN_DATABASE) {
+        $row[] = t('Normal (!export)', array('!export' => l(t('export'), 'admin/settings/mollom/export/' . $form_id)));
+      }
+      else if ($mollom_form['export_type'] & EXPORT_IN_CODE) {
+        $row[] = t('Default');
+      }

1) minor: Can we rename 'export_type' to 'storage'?

2) major: It seems like I cannot unprotect or disable a form protection that lives in code? Or do I get this code wrong?

+++ mollom.install 18 Feb 2010 06:32:09 -0000
@@ -126,6 +126,16 @@ function mollom_schema() {
     'primary key' => array('form_id'),
+    'export' => array(
+      'key' => 'form_id',
+      'identifier' => 'mollom_form',

"key" and "identifier" seem to duplicate schema information - are they required?

+++ mollom.module 18 Feb 2010 06:32:10 -0000
@@ -528,13 +548,40 @@ function mollom_get_form_info($form_id =
+function mollom_form_load_all() {
...
+      $mollom_forms[] = $form_id;
...
+    $result = db_query("SELECT form_id FROM {mollom_form}");
+    while ($form_id = db_result($result)) {
+      $mollom_forms[] = $form_id;
...
+  return $mollom_forms;

Hm. It's a bit strange that this function is named mollom_form_load_all() and lives next to mollom_form_load(), but returns something completely different than mollom_form_load().

Powered by Dreditor.

yhahn’s picture

Status:Needs work» Needs review
StatusFileSize
new11.58 KB
PASSED: [[SimpleTest]]: [MySQL] 1,289 pass(es).
[ View ]

Thanks for the review. New version of patch attached.

  • Can we do a single $exportable = module_exists('ctools');
    • Yes
  • 1) minor: Can we rename 'export_type' to 'storage'?
    • Short answer: no. Long answer: CTools uses this key for this metadata by default, we could make a copy of the information on load but it wouldn't really make sense to.
  • 2) major: It seems like I cannot unprotect or disable a form protection that lives in code? Or do I get this code wrong?
    • I updated the patch to take advantage of CTools built-in ->disabled flag. Default or Overridden forms can now be disabled (behaves the same as an unprotected form).
  • "key" and "identifier" seem to duplicate schema information - are they required?
    • I've removed identifier which defaults to the name of the table. While CTools could probably derive key in many cases from the primary key of a table, it currently doesn't handle this so we must declare it.
  • It's a bit strange that this function is named mollom_form_load_all() and lives next to mollom_form_load(), but returns something completely different
    • Revised to return an array of loaded mollom forms.
sun’s picture

Thank you! This looks pretty good now. I'll try to come back to this issue and actually test the code before approving it.

Speaking of tests, we probably need a new test case for this feature, which leverages the new 'dependencies' property in getInfo() for CTools, and ensures that form protections can be defined in code, disabled/enabled, and overridden.

sun’s picture

Status:Needs review» Needs work

After applying this patch, I get the following PHP notices + warnings on the front page:

* Notice: Undefined index: name in ctools_export_load_object() (line 122 of sites\all\modules\ctools\includes\export.inc).

* User warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'type for db_type_placeholder)' at line 1 query: SELECT * FROM mollom_form WHERE name IN (unsupported type for db_type_placeholder) in _db_query() (line 128 of includes\database.mysqli.inc).

* Notice: Undefined variable: mollom_form in mollom_form_load() (line 643 of sites\all\modules\mollom-DRUPAL-6--1\mollom.module).

The first one looks like I need to update ctools on my Mollom test site - do we require a certain minimum API version of CTools?

sun’s picture

After updating CTools to latest CVS DRUPAL-6--1, those notices and warnings seem to disappear.

However, exporting a form protection defines:

$mollom_form = new stdClass;
$mollom_form->disabled = FALSE; /* Edit this to true to make a default mollom_form disabled initially */
$mollom_form->api_version = 1;
$mollom_form->form_id = 'comment_form';
$mollom_form->mode = TRUE;
$mollom_form->enabled_fields = array(
  '0' => 'subject',
  '1' => 'comment',
);
$mollom_form->module = 'comment';

$mollom_form->mode is not a boolean, but instead one of the constants MOLLOM_MODE_DISABLED, MOLLOM_MODE_CAPTCHA, or MOLLOM_MODE_ANALYSIS.

sun’s picture

StatusFileSize
new12.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch mollom-DRUPAL-6--1.export.7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

- Fixed some minor issues.
- Additionally implemented Bulk exporter support to be able to actually test this more easily.

Remaining issues:
- After exporting a form into code, it is properly displayed as such in the form administration. However, after clicking on "revert", the form suddenly appears at the end of the list.

- A disabled form cannot be enabled. (link points to 'disable')

EDIT: The 'mode' bug mentioned in #6 also remains.

DamienMcKenna’s picture

There are two tables:

Given it has been eight months since the last update, and CTools has greatly improved the exportables support, could we just start with some basic exporting via the extensions on hook_schema and build from there through multiple releases?

DamienMcKenna’s picture

StatusFileSize
new802 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch mollom-n717874-9.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Here's the bare hook_schema() for 'mollom_form' extracted from the patches above for D6.

HongPong’s picture

subscribe - any luck with this?

Dave Reid’s picture

Version:6.x-1.x-dev» 7.x-2.x-dev

Bumping this to 7.x-2.x as this would require an additional dependency.

sun’s picture

To clarify, we're not going to add a hard dependency on another module. However, we can implement optional support for ctools - which, I think, would still look similar to #7 (although that's a lot of code for this functionality and I seriously hope we can cut it down). I'm not sure what kind of effect a schema-definition-only patch like #9 would have?

DamienMcKenna’s picture

Nobody has worked on this further?

Garrett Albright’s picture

StatusFileSize
new12.85 KB
FAILED: [[SimpleTest]]: [MySQL] 106 pass(es), 58 fail(s), and 2 exception(es).
[ View ]

My mostly brute force attempt at a D7 version of the patch. Should apply, but will cause a "unsupported operand" error that I'm not sure how to fix without being more familiar with what the codebase looked like in the past (and possibly other errors when that one is fixed), but I'm hoping this will provide a kick in the pants for this to be picked up again.

Garrett Albright’s picture

StatusFileSize
new12.14 KB
FAILED: [[SimpleTest]]: [MySQL] 2,876 pass(es), 3 fail(s), and 0 exception(es).
[ View ]

Powered through it some more. No more fatal errors. In fact, I think this might even… work.

Garrett Albright’s picture

Status:Needs work» Needs review
StatusFileSize
new11.99 KB
FAILED: [[SimpleTest]]: [MySQL] 2,876 pass(es), 3 fail(s), and 0 exception(es).
[ View ]

Fix whitespace complaints when patch is applied. Also, NR.

Status:Needs review» Needs work

The last submitted patch, 717874-16-exportables-for-mollom.patch, failed testing.

Garrett Albright’s picture

StatusFileSize
new11.89 KB
FAILED: [[SimpleTest]]: [MySQL] 4,226 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Fix some lingering D6 menu paths and such.

Boobaa’s picture

Status:Needs work» Needs review

Let's have a bot review.

Status:Needs review» Needs work

The last submitted patch, 717874-18-exportables-for-mollom.patch, failed testing.

adamdicarlo’s picture

The patch doesn't change any tests, so that's probably why they're failing, right? Since the admin forms have changed?

itaine’s picture

#18 applies cleanly and works on 7.x-2.1 but needs to be rerolled for 2.2 and committed.

cam8001’s picture

Status:Needs work» Needs review
StatusFileSize
new12.52 KB
PASSED: [[SimpleTest]]: [MySQL] 4,371 pass(es).
[ View ]

Here's a reroll against 7.x.2.x/7.x-2.2.

itaine’s picture

Is it me or the protection mode keeps reverting back to CAPTCHA? When I set a form definition to Text analysis and then export it into a feature, upon rebuild it pops back up set on CAPTCHA. Driving me type wonkers. Even when I blow away the mollom.inc manually and re-export a clean one... still no joy.

Can anyone else recreate?

edit:
Confirmed, protection mode isn't exporting.

patty.fresonke’s picture

Does the patch in #23 work for 7.x-2.3? Just wondering if anyone tried it...

Downloaded 7.x-2.2 just to try the patch...and I get these errors:
Notice: Undefined index: module in mollom_admin_form_list() (line 37 of /home/pfresonke/public_html/public_html/sites/all/modules/contrib/mollom/mollom.admin.inc).
Notice: Undefined index: mode in mollom_admin_form_list() (line 54 of /home/pfresonke/public_html/public_html/sites/all/modules/contrib/mollom/mollom.admin.inc).
Notice: Undefined index: export_type in mollom_admin_form_list() (line 70 of /home/pfresonke/public_html/public_html/sites/all/modules/contrib/mollom/mollom.admin.inc).

Plus a bunch more of the same on different lines...anyone else run into this?

netsensei’s picture

You've also left some trailing whitespace around line 341 in this patch.

+++ b/mollom.moduleundefined
@@ -1042,6 +1134,7 @@ function mollom_form_load($form_id) {
   }
+  ¶
   return $mollom_form;
}
netsensei’s picture

Status:Needs review» Needs work

#25: I don't think this code applies cleanly for 2.3. You'll need to backport parts of it to make it all fit. It does apply to the 2.x-dev branch as intended. The issue reported in #24 does warrant further investigation though.

netsensei’s picture

Also. You can only export a configuration once. When it is already exported, there is no way to export the overridden configuration again. You are only able to
revert back to the original exported configuration.

I think we need to split the two the overridden configuration + the export functionality.

I'm also wary about the navigational path from the exported code preview back to the list of forms. The breadcrumb doesn't seem to follow nor is there a "back to form configurations" button and/or link.

netsensei’s picture

Status:Needs work» Needs review
StatusFileSize
new12.08 KB
PASSED: [[SimpleTest]]: [MySQL] 4,883 pass(es).
[ View ]

And a reroll against 7.x-2.x. This patch:

* Removes the whitespace error
* UI change: the export functionality is now always available.
* UI change: I've recycled the "Database overrides code", "In code" or "In database" flags which are also used in the Views UI.

netsensei’s picture

I've also discovered the cause of the issue reported in #717874-24: Provide exportables for Mollom forms. The protection mode is stored in a tinyint type db field. ctools_export_object does not make a distinction between tinyint values and boolean values. The function turns any tinyint value (except 0) into a TRUE. It's a CTools issue reported here: #1760752: Tiny int should not be exported as a boolean

I'm going to reroll the patch with a field specific export callback to get around this problem.

The other solution would be to change the field type size from tiny to small. I don't favor changing the schema here because of a code issue in external API module though.

netsensei’s picture

StatusFileSize
new12.53 KB
PASSED: [[SimpleTest]]: [MySQL] 4,884 pass(es).
[ View ]

Okay. Patch should fix the mismatch problem. Go testbot!

chriscohen’s picture

Thanks for the work so far, but I've installed the patch from #31, made some changes, and tried to revert my feature. The revert doesn't do anything at all; it says overridden, and there is no green status text at the top of the page like there usually is during a revert!

netsensei’s picture

Where did you make your changes? In the code or in the database (configuration)?

chriscohen’s picture

So the changes were made on a local copy in the UI, and then the feature was downloaded. The code was deployed to a production system and the feature was reverted, but this is where the issue came in, as the revert apparently had no effect.

Closer inspection using the diff module reveals that the left side of the diff doesn't have anything at all in it, just the word 'FALSE', which would be an obvious reason for the revert to fail, but I can't seem to see why the left side of the diff would be 'FALSE'.

scor’s picture

Check these two things:
1. this patch is against mollom 7.x-2.x, make sure to upgrade to 7.x-2.4 if you are still using a 7.x-1.x version
2. Make sure you have the following code in your MYFEATURE.features.inc:

  list($module, $api) = func_get_args();
  if ($module == "mollom" && $api == "mollom") {
    return array("version" => "1");
  }

For the records, the patch #31 works for me with 7.x-2.4.

netsensei’s picture

If #34 can confirm #35, can we put this to RTBC?

chriscohen’s picture

This is #34. I can confirm #35!

netsensei’s picture

Status:Needs review» Reviewed & tested by the community

Putting this into RTBC. :-)

esolano’s picture

Version:7.x-2.x-dev» 7.x-2.4

Hi, I'm having the same issue as #34. I'm on version 7.x-2.4
Looking at the [my_module].mollom.inc file, it is implementing this hook: hook_default_mollom_form() but when I search for it, I can't find it. Maybe that's causing the issue?

Regards.

stella’s picture

Version:7.x-2.4» 7.x-2.6

Can confirm that the patch still works with 7.x-2.6

stella’s picture

Status:Reviewed & tested by the community» Needs work

Update: while the configuration gets deployed, and caches cleared, etc, the captcha does not appear until the settings are saved to the database for each configured form :(

stella’s picture

Status:Needs work» Needs review
StatusFileSize
new13.09 KB
PASSED: [[SimpleTest]]: [MySQL] 6,710 pass(es).
[ View ]

Patch re-roll against 7.x-2.6 and also fixes the issue I described in #42. Basically the problem was that mollom_form_cache() was still only loading from the database and not looking at the versions in code. I changed it so it now calls mollom_form_load_all() instead.

yukare’s picture

Please, can someone update this patch for 2.7 and provide instructions how to use it?

chriscohen’s picture

Version:7.x-2.6» 7.x-2.7

+1 to #44. I applied the patch from #42 and it worked on 2.7. Can we get this into a release, please? Happy to mark this RTBC.

mariacha1’s picture

#44 works when you I the admin interface, but it's failing for me when I try to update a feature with drush fu.

jkswoods’s picture

The patch works on #42 for 2.7 and no higher - so it was unable to patch with 2.8

mr.york’s picture

Version:7.x-2.7» 7.x-2.10
Issue summary:View changes
StatusFileSize
new13.6 KB
PASSED: [[SimpleTest]]: [MySQL] 7,463 pass(es).
[ View ]

Reroll.

mr.york’s picture

Version:7.x-2.10» 7.x-2.x-dev
mr.york’s picture

eshta’s picture

I'm testing right now locally with an eye to get this committed into the main dev line. This is fantastic work! This is still missing some tests, however, to ensure that the right links display when ctools is enabled and that the new form loader method returns expected database + ctools defined forms. After that (and assuming it tests out for me as well as it has for everyone else) I see no reason that we can't get this in.

eshta’s picture

Status:Needs review» Needs work
netsensei’s picture

StatusFileSize
new18.76 KB
FAILED: [[SimpleTest]]: [MySQL] 7,498 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new5.45 KB

I've added a separate test class MollomTestingExportingFormConfiguration that tests exporting / overriding / reverting of form configuration. See: interdiff.txt for the details.

Things I've noticed so far:

1/
How do we deal with ctools in our tests? It's not a dependency of Mollom and the patch does a module_exists check. Do we go with the same approach in our tests?

2/
Trying to configuring an exported form configuration that was originally not created on the site, triggers a test failure. While importing the form configuration, no entry is made in the mollom_forms table. setProtectionUI() assumes we need to create a new form configuration instead of editing an existing form configuration, but it fails because 'mollom_exported_basic_test_form' is not in the list of available forms to configure.

Needs more work.

netsensei’s picture

Status:Needs work» Needs review

Go testbot!

Status:Needs review» Needs work

The last submitted patch, 52: 717874-52-exportables-for-mollom.patch, failed testing.

netsensei’s picture

1/

Realised that the patch contains a mollom_form_load_all() function which should be set in setProtectionUi() It only returns the exported forms, not forms in the db when called from the tests.

2/
Also, the 'unprotect' link on exported forms doesn't make a lot of sense imho.

'Unprotecting' will remove the form from the list, but it's still exported through hook_default_mollom_form(). Clearing caches won't execute that hook and in Features it will still read 'Default'.

I'd say: let's disable unprotecting for exported forms.

netsensei’s picture

Okay. Scratch 2/ from #52. Noticed that exported forms don't have an "unprotect" link.

netsensei’s picture

Status:Needs work» Needs review
StatusFileSize
new20.75 KB
FAILED: [[SimpleTest]]: [MySQL] 7,498 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new4.31 KB

Changes:

1/ setProtectionUI() now makes the distinction between non-existing forms, forms stored in the database and forms stored in code.
2/ Added extra tests against updating (overriding) exported forms not stored in the database (adding them)
3/ Added extra tests against updating stored (already overridden) exported forms
4/ Added extra tests against reverting overridden forms

Re: 1/: mollom_form_load_all returns give me all objects back, but somehow, neither their export_type or in_code_only properties are set correctly during testing. Not that it matters since I had to retain the SQL query to avoid a hard dependency against CTools.

Go testbot!

Status:Needs review» Needs work

The last submitted patch, 57: 717874-57-exportables-for-mollom.patch, failed testing.

netsensei’s picture

The last submitted patch, 57: 717874-57-exportables-for-mollom.patch, failed testing.

netsensei’s picture

That's odd. Applying the patch locally to the 7.x-2.x branch works perfectly. I don't understand why it should fail with a cryptic invalid permission "administer mollom" failure on setUp(). Anyone has any ideas?

netsensei’s picture

Status:Needs work» Needs review
StatusFileSize
new20.58 KB
FAILED: [[SimpleTest]]: [MySQL] 7,498 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new499 bytes

Let's see how this fares. I've removed the admin_user being created in the test.
Patch doesn't apply cleanly yet. Let's make it work first before fixing whitespace errors and the like.

Status:Needs review» Needs work

The last submitted patch, 62: 717874-62-exportables-for-mollom.patch, failed testing.

netsensei’s picture

StatusFileSize
new20.57 KB
FAILED: [[SimpleTest]]: [MySQL] 7,498 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new422 bytes

Getting rid of the mollom module in setUp().

netsensei’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 64: 717874-64-exportables-for-mollom.patch, failed testing.

netsensei’s picture

Status:Needs work» Needs review
StatusFileSize
new20.81 KB
FAILED: [[SimpleTest]]: [MySQL] 7,498 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new968 bytes

Status:Needs review» Needs work

The last submitted patch, 67: 717874-67-exportables-for-mollom.patch, failed testing.

netsensei’s picture

Status:Needs work» Needs review
StatusFileSize
new457 bytes
new21.25 KB
FAILED: [[SimpleTest]]: [MySQL] 7,498 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Hm. Me thinks this has something to do with the testbot not resolving ctools as a dependency. Next attempt by adding test_dependencies to the main mollom.info file per https://drupal.org/node/542202 (it's a soft dependency: only testbot should need this)

Status:Needs review» Needs work

The last submitted patch, 69: 717874-69-exportables-for-mollom.patch, failed testing.

sun’s picture

Optional test dependencies need to be specified in the following way:

In the .info file: (only affects which projects are checked out by d.o testbots)

test_dependencies[] = ctools

In affected test classes: (consumed by Simpletest to determine whether requirements of a test class are met)

<?php
function getInfo() {
  return array(
    ...
   
'dependencies' => array('ctools'),
  );
}
?>

And of course, in the setUp() method of affected test classes: (to actually install the dependency)

<?php
function setUp() {
 
parent::setUp(array('ctools'));
}
?>

Note that the test_dependencies change to the .info file might have to be committed separately upfront, since the .info metadata of a project may be globally cached and not re-parsed for every test. (cf. https://drupal.org/project/project_dependency)

netsensei’s picture

Status:Needs work» Needs review
StatusFileSize
new21.06 KB
PASSED: [[SimpleTest]]: [MySQL] 7,498 pass(es).
[ View ]
new1 KB

Thx Sun! :-) So I got it nearly right.

I've added the dependencies property in getInfo() and removed most of setUp() since the default configuration should be suitable for this particular test class.

If the .info metadata needs to be committed upfront: do I create a separate patch/issue for that?

eshta’s picture

Status:Needs review» Needs work

Looks like the info file doesn't need an upfront commit based on the tests passing here. I think we're just down to the nit-picky white-space clean-up (take a look at Dreditor for violations). All of the testing on my local machine is working out well ;-)

This will be a really exciting addition.

netsensei’s picture

Status:Needs work» Needs review
StatusFileSize
new7.12 KB
new21.3 KB
PASSED: [[SimpleTest]]: [MySQL] 7,498 pass(es).
[ View ]

Okay. Got rid of the whitespace. Go testbot!

eshta’s picture

Status:Needs review» Reviewed & tested by the community

eshta’s picture

Version:7.x-2.x-dev» 6.x-2.x-dev
Issue tags:+Needs backport to 6.x

Committed to d7 (albeit with a messed up commit message)....
Backport to d6 anyone?

eshta’s picture

While this worked great with the d.o. testbots, I can't seem to get these tests to pass locally at all.

Chaulky’s picture

What kind of errors are you getting? How are you serving the site locally?

I've had false failures before because there was no /etc/hosts entry redirecting the localhost IP to the local site's domain name.

eshta’s picture

All of the Mollom tests run fine and pass for me except the new exportables tests. They always fail in the setup with a 'Class 'MollomDrupalTest' not found' exception. I do have ctools module installed and available on this test site. Other than ctools there isn't anything different in the setup method that I can see that should cause a failure.

eshta’s picture

Version:6.x-2.x-dev» 7.x-2.x-dev
Status:Reviewed & tested by the community» Needs work

Hmmm - so if I can piece this together it looks like the new exportables test was never run for this patch and therefore it never fails.
https://qa.drupal.org/pifr/test/806308

One the .info commit was made (with this patch) then the Drupal testbots picked up on the new test and it started failing.
https://qa.drupal.org/pifr/test/826028

So I guess the correct answer in #73 was that yes - we should pre-commit the .info change and then run the tests.

I'm going to have to doctor up the commit history and revert this code but leave the info change so that we can proceed to figuring out what is wrong with the test. At this point these failures are also blocking some other issues that are RTBC.

I should have the dev branch squared away within the next hour or so.

  • eshta committed 99693d4 on 7.x-2.x authored by netsensei
    Issue #717874 by netsensei, yhahn, DamienMcKenna, Garrett Albright:...
  • eshta committed c2c16c2 on 7.x-2.x
    Revert
    
    This reverts commit for Issue #717874 - Mollom exportables...
eshta’s picture

Status:Needs work» Needs review
StatusFileSize
new21.1 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to create dependency directory.
[ View ]

Attaching a re-rolled patch after the .info change commit for testbots.

Status:Needs review» Needs work

The last submitted patch, 83: 717874-83.mollom.exportables.patch, failed testing.

Aron Novak’s picture

StatusFileSize
new21.1 KB
FAILED: [[SimpleTest]]: [MySQL] 7,582 pass(es), 12 fail(s), and 1 exception(s).
[ View ]
new21.1 KB
FAILED: [[SimpleTest]]: [MySQL] 7,582 pass(es), 12 fail(s), and 1 exception(s).
[ View ]

Rerolled to apply cleanly to 7.x-2.12 and the 7.x-7.x.

eshta’s picture

Status:Needs work» Needs review

The last submitted patch, 85: 717874-84.mollom.exportables-7.x-2.x.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 85: 717874-84.mollom.exportables-7.x-2.12.patch, failed testing.

eshta’s picture

@Aron Novak Thanks for the re-rolls, but the problem appears to be with the failed exportables test. Once that test is resolved I'm happy to commit this.

Aron Novak’s picture

Strange, i applied the patch at localhost (for the -dev) and all the tests were successful. Can you try it out on your side as well to confirm it's a testbot specific thing?

Aron Novak’s picture

StatusFileSize
new20.86 KB

And this patch applies to the downloadable 7.x-2.12 package.

eshta’s picture

I had the same failure locally as well. Maybe try with and without the ctools module installed?

sk33lz’s picture

The patch in #91 also applies cleanly to mollom-7.x-2.13 with some minor offsets. It exports the data properly to features and those settings are updated from local to dev in my manual tests.

eshta’s picture

sk33lz - did you run the automated tests? Did they pass with and without ctools installed? That's my only remaining concern with this.