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.
Comment | File | Size | Author |
---|---|---|---|
#104 | 717874-104.mollom.exportables-7.x-2.14.patch | 20.62 KB | Kpolymorphic |
#100 | provide_exportables_for-717874-100.patch | 20.67 KB | alansaviolobo |
#95 | mollom-exportables-717874-95.patch | 19.5 KB | Boobaa |
#91 | 717874-91.mollom.exportables-7.x-2.12.patch | 20.86 KB | Aron Novak |
#85 | 717874-84.mollom.exportables-7.x-2.12.patch | 21.1 KB | Aron Novak |
Comments
Comment #2
sunThanks! That test failure is caused by a different bug unrelated this patch, #716250: Blacklisting test
Can we do a single
$exportable = module_exists('ctools');
at the beginning?
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?
"key" and "identifier" seem to duplicate schema information - are they required?
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.
Comment #3
yhahn CreditAttribution: yhahn commentedThanks for the review. New version of patch attached.
->disabled
flag. Default or Overridden forms can now be disabled (behaves the same as an unprotected form).identifier
which defaults to the name of the table. While CTools could probably derivekey
in many cases from the primary key of a table, it currently doesn't handle this so we must declare it.mollom_form_load_all()
and lives next tomollom_form_load()
, but returns something completely differentComment #4
sunThank 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.
Comment #5
sunAfter applying this patch, I get the following PHP notices + warnings on the front page:
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?
Comment #6
sunAfter updating CTools to latest CVS DRUPAL-6--1, those notices and warnings seem to disappear.
However, exporting a form protection defines:
$mollom_form->mode is not a boolean, but instead one of the constants MOLLOM_MODE_DISABLED, MOLLOM_MODE_CAPTCHA, or MOLLOM_MODE_ANALYSIS.
Comment #7
sun- 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.
Comment #8
DamienMcKennaThere 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?
Comment #9
DamienMcKennaHere's the bare hook_schema() for 'mollom_form' extracted from the patches above for D6.
Comment #10
HongPong CreditAttribution: HongPong commentedsubscribe - any luck with this?
Comment #11
Dave ReidBumping this to 7.x-2.x as this would require an additional dependency.
Comment #12
sunTo 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?
Comment #13
DamienMcKennaNobody has worked on this further?
Comment #14
Garrett Albright CreditAttribution: Garrett Albright commentedMy 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.
Comment #15
Garrett Albright CreditAttribution: Garrett Albright commentedPowered through it some more. No more fatal errors. In fact, I think this might even… work.
Comment #16
Garrett Albright CreditAttribution: Garrett Albright commentedFix whitespace complaints when patch is applied. Also, NR.
Comment #18
Garrett Albright CreditAttribution: Garrett Albright commentedFix some lingering D6 menu paths and such.
Comment #19
BoobaaLet's have a bot review.
Comment #21
adamdicarlo CreditAttribution: adamdicarlo commentedThe patch doesn't change any tests, so that's probably why they're failing, right? Since the admin forms have changed?
Comment #22
itaine CreditAttribution: itaine commented#18 applies cleanly and works on 7.x-2.1 but needs to be rerolled for 2.2 and committed.
Comment #23
Cameron Tod CreditAttribution: Cameron Tod commentedHere's a reroll against 7.x.2.x/7.x-2.2.
Comment #24
itaine CreditAttribution: itaine commentedIs 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.
Comment #25
patty.fresonke CreditAttribution: patty.fresonke commentedDoes 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?
Comment #26
netsensei CreditAttribution: netsensei commentedYou've also left some trailing whitespace around line 341 in this patch.
Comment #27
netsensei CreditAttribution: netsensei commented#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.
Comment #28
netsensei CreditAttribution: netsensei commentedAlso. 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.
Comment #29
netsensei CreditAttribution: netsensei commentedAnd 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.
Comment #30
netsensei CreditAttribution: netsensei commentedI'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 booleanI'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.
Comment #31
netsensei CreditAttribution: netsensei commentedOkay. Patch should fix the mismatch problem. Go testbot!
Comment #32
chriscohen CreditAttribution: chriscohen commentedThanks 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!
Comment #33
netsensei CreditAttribution: netsensei commentedWhere did you make your changes? In the code or in the database (configuration)?
Comment #34
chriscohen CreditAttribution: chriscohen commentedSo 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'.
Comment #35
scor CreditAttribution: scor commentedCheck 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:
For the records, the patch #31 works for me with 7.x-2.4.
Comment #36
netsensei CreditAttribution: netsensei commentedIf #34 can confirm #35, can we put this to RTBC?
Comment #37
chriscohen CreditAttribution: chriscohen commentedThis is #34. I can confirm #35!
Comment #38
netsensei CreditAttribution: netsensei commentedPutting this into RTBC. :-)
Comment #39
esolano CreditAttribution: esolano commentedHi, 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.
Comment #40
stella CreditAttribution: stella commentedCan confirm that the patch still works with 7.x-2.6
Comment #41
stella CreditAttribution: stella commentedUpdate: 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 :(
Comment #42
stella CreditAttribution: stella commentedPatch 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.
Comment #43
yukare CreditAttribution: yukare commentedPlease, can someone update this patch for 2.7 and provide instructions how to use it?
Comment #44
chriscohen CreditAttribution: chriscohen commented+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.
Comment #45
mariacha1 CreditAttribution: mariacha1 commented#44 works when you I the admin interface, but it's failing for me when I try to update a feature with
drush fu
.Comment #46
jkswoods CreditAttribution: jkswoods commentedThe patch works on #42 for 2.7 and no higher - so it was unable to patch with 2.8
Comment #47
mr.york CreditAttribution: mr.york commentedReroll.
Comment #48
mr.york CreditAttribution: mr.york commentedComment #49
mr.york CreditAttribution: mr.york commented47: 717874-47-exportables_for_mollom.patch queued for re-testing.
Comment #50
eshta CreditAttribution: eshta commentedI'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.
Comment #51
eshta CreditAttribution: eshta commentedComment #52
netsensei CreditAttribution: netsensei commentedI'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.
Comment #53
netsensei CreditAttribution: netsensei commentedGo testbot!
Comment #55
netsensei CreditAttribution: netsensei commented1/
Realised that the patch contains a
mollom_form_load_all()
function which should be set insetProtectionUi()
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.
Comment #56
netsensei CreditAttribution: netsensei commentedOkay. Scratch 2/ from #52. Noticed that exported forms don't have an "unprotect" link.
Comment #57
netsensei CreditAttribution: netsensei commentedChanges:
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!
Comment #59
netsensei CreditAttribution: netsensei commented57: 717874-57-exportables-for-mollom.patch queued for re-testing.
Comment #61
netsensei CreditAttribution: netsensei commentedThat'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?
Comment #62
netsensei CreditAttribution: netsensei commentedLet'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.
Comment #64
netsensei CreditAttribution: netsensei commentedGetting rid of the mollom module in setUp().
Comment #65
netsensei CreditAttribution: netsensei commentedComment #67
netsensei CreditAttribution: netsensei commentedComment #69
netsensei CreditAttribution: netsensei commentedHm. 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)
Comment #71
sunOptional 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)
In affected test classes: (consumed by Simpletest to determine whether requirements of a test class are met)
And of course, in the setUp() method of affected test classes: (to actually install the dependency)
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)
Comment #72
netsensei CreditAttribution: netsensei commentedThx Sun! :-) So I got it nearly right.
I've added the dependencies property in
getInfo()
and removed most ofsetUp()
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?
Comment #73
eshta CreditAttribution: eshta commentedLooks 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.
Comment #74
netsensei CreditAttribution: netsensei commentedOkay. Got rid of the whitespace. Go testbot!
Comment #75
eshta CreditAttribution: eshta commentedComment #77
eshta CreditAttribution: eshta commentedCommitted to d7 (albeit with a messed up commit message)....
Backport to d6 anyone?
Comment #78
eshta CreditAttribution: eshta commentedWhile this worked great with the d.o. testbots, I can't seem to get these tests to pass locally at all.
Comment #79
Chaulky CreditAttribution: Chaulky commentedWhat 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.
Comment #80
eshta CreditAttribution: eshta commentedAll 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.
Comment #81
eshta CreditAttribution: eshta commentedHmmm - 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.
Comment #83
eshta CreditAttribution: eshta commentedAttaching a re-rolled patch after the .info change commit for testbots.
Comment #85
Aron NovakRerolled to apply cleanly to 7.x-2.12 and the 7.x-7.x.
Comment #86
eshta CreditAttribution: eshta commentedComment #89
eshta CreditAttribution: eshta commented@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.
Comment #90
Aron NovakStrange, 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?
Comment #91
Aron NovakAnd this patch applies to the downloadable 7.x-2.12 package.
Comment #92
eshta CreditAttribution: eshta commentedI had the same failure locally as well. Maybe try with and without the ctools module installed?
Comment #93
sk33lz CreditAttribution: sk33lz commentedThe 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.
Comment #94
eshta CreditAttribution: eshta commentedsk33lz - did you run the automated tests? Did they pass with and without ctools installed? That's my only remaining concern with this.
Comment #95
BoobaaRerolled the working patch from #85 against latext 2.x-dev; haven't run the tests though.
Comment #96
BoobaaComment #99
pradeep22saini CreditAttribution: pradeep22saini commentedI used the patch #74 for exporting and importing form configurations it worked for me.
Comment #100
alansaviolobo CreditAttribution: alansaviolobo at Axelerant commentedrerolling patch against latest dev.
Comment #102
Kpolymorphic CreditAttribution: Kpolymorphic at GollyGood Software commentedRe-rolled for 7.x-2.14
Main change was to remove fix to comment formatting that was causing patch failure (due to fix being committed already in 7.x-2.14)
Comment #103
Kpolymorphic CreditAttribution: Kpolymorphic at GollyGood Software commentedComment #104
Kpolymorphic CreditAttribution: Kpolymorphic at GollyGood Software commentedRe-rolled previous patch, it acquired wrong line numbers somehow... sorry.
Comment #105
Kpolymorphic CreditAttribution: Kpolymorphic at GollyGood Software commentedThis worked fine in my test, didnt intend to change the status.