exportables_sync() should be tied to enabling/disabling the module providing the default items

Hello!

I think I see what you're doing now. Had to take a second look through the code at the end of exportables_sync(); I had seen that you were caching defaults, but I don't think I understood the purpose.

In the meantime, I really like the idea of giving the user more control over when the sync happens (in addition to doing it automatically whenever modules are enabled or disabled)…to that end, I've attached another patch incorporating a couple of administrative interfaces. The user can now turn sync-on-init on or off based on their preferences, and they can also sync specific exportables types at will.

I haven't committed this to CVS or anything yet; figured it'd be worth a little review beforehand, plus there's still the automatic enable/disable synching to do before this particular issue is fixed. Working on that now.

(EDIT: I should note that this patch changes the default sync-on-init behavior; it has to be enabled by the end user. On a production release I wouldn't have introduced that kind of change, but I figure since this is still very much in development, it'd be OK to patch with the end goal in mind rather than preserving backwards compatibility.)

Thanks!
Adam

I found the duplication error; was a whole lot simpler than I'd thought. As it turns out, the db_query() call that deleted the records didn't have all its arguments specified. Went ahead and fixed it in CVS, should be in the next dev release.

Meanwhile, there are still some things to do in this ticket; I've attached an updated version of my admin_settings patch against the current HEAD, and will continue to work on the rest of this next week as time permits.

Thanks!
Adam

Went ahead and committed the previous patch. The number of changes necessary to resolve this particular issue is pretty high, and rather than roll them all into one huge patch full of loosely-related changes, I'd rather isolate each piece and commit individually. Lots easier to review and test each stage that way.

There's still more to do on this issue, of course; I'm going to try to abstract some of the exportables_sync() logic out into separate reusable functions so that it'll be easier to write the _remove_defaults_from_database() functionality that will be called when modules are disabled; should have a patch for that soon.

Thanks!
Adam

OK …I've got my patch put together, and this one's a bit of a doozy. There are two basic updates included:

1. Lots of the functionality in exportables_sync() has been abstracted out into atomic, reusable functions; this makes the sync() function a bit easier to read _and_ makes it possible to benefit from parts of its logic in other functions.
2. There is now also an exportables_unsync(), complete with admin interface for testing purposes. It removes default items from the database, and is intended to be used whenever a module providing defaults is disabled.

I haven't yet put the latter function to use; that's the last piece of the puzzle. Should also be pretty easy to implement, but I wanted to get this material out there for review today, since it'll be a little while before I can work on the rest of it. Please have a look!

Thanks,
Adam

Hello!

Just realized something kind of dumb, and wanted to make a note of it here… when exportables_sync() and exportables_unsync() are tied in to the module enable/disable process, they will need to sync/unsync _only_ those exportable default items provided by that particular module. In other words, if you've got two modules that both provide some default "taxonomy_vocabulary" items and you disable Module A, you don't want Module B's taxonomy_vocabulary items to get deleted.

Worth mentioning, because the previous patch doesn't allow for this. Might need to write an exportables_sync_by_module() too.

Thanks!
Adam

OK, got an updated patch that includes the ability to sync/unsync by module as well as type. Still not actually doing this on module enable/disable, but all the functionality we'll need is in this patch.

Thanks!
Adam

@makara thanks for the feedback; I've adjusted the patch accordingly.

Thanks!
Adam

Hello!

OK, the patch is committed, and should appear in the next nightly development build.

All that's left for this issue is to actually automatically run exportables_sync() when a module providing exportables is enabled, and exportables_unsync() when a module providing exportables is disabled. Various approaches to this have already been discussed earlier in the thread; I'll be posting some code soon to make it happen. If it turns out to be good code, we'll be able to close this issue!

@makara …I'm not sure what you mean; while features integration is the primary use case for exportables, I think the original intention was that other, non-features-based modules should be able to use it as well as desired. I do think that the bulk of our development work on this module should focus on features integration, but I don't know that we ought to introduce it as a strict dependency at this point.

Thanks!
Adam

Hello!

All right, I've gone ahead and put together what I believe is the simplest approach to the sync-on-enable problem given that the primary use case here is features integration.

This patch is extremely short; all it does is ensure that when exportables is providing the export hook for a particular type, it also provides enable/disable hooks for the generated feature module; these hooks run exportables_sync() and exportables_unsync(), respectively.

I went with this approach rather than hooking into the system_modules form primarily for simplicity. I did spend some time looking into how that would work, but it looked like it would be pretty difficult to detect which modules were enabled/disabled, and also to guarantee that the appropriate syncing functions were run whenever a module was enabled via some means other than the standard system_modules form (drush, features, etc.).

My solution works perfectly for modules generated by Features; however, the drawback of this approach is that it requires a little more awareness on the part of developers writing modules that aren't generated by Features…they'd need to write the enable/disable hooks themselves. This is pretty simple to do…just one line per hook:

function mymodule_enable() {
  exportables_sync('mytype', 'mymodule');
}

function mymodule_disable() {
  exportables_unsync('mytype', 'mymodule');
}

Anyway, the patch is attached, so you can see how I'm automatically generating this for features-generated modules. Please let me know what you think; I'd especially like to see this reviewed by someone deeply familiar with the Features API, just in case I'm using it wrong.

Thanks!
Adam

OK, I've gone ahead and committed the latest patch, which I believe fixes all the remaining bits of this issue. Exported features will have sync-on-enable and unsync-on-disable for free, and other types of modules can implement it very easily with the functions described in #12.

Thanks!
Adam