Import/Export Support
sirkitree - November 17, 2008 - 17:35
| Project: | Flag |
| Version: | 6.x-1.x-dev |
| Component: | Flag core |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
Description
This is an offshoot request from #330141: Allow modules to expose default flags.
Since we'll have the ability to have default flags defined by other modules, it would be nice to also have a way to export these definitions.
1, it'll help with testing of the aforementioned feature. 2, views provides this as they foresaw the need to move default views around to multiple systems. (I'm just assuming that was where the idea originated.)
#1
We should definitely consider to migrate the code for default flags to integrate with and leverage from CTools, which provides an API for "exportables" already.
#2
Yeah I've been thinking about the same thing. The unfortunate thing is CTools currently is specific to the way Panels/Views operate, where the entire object is overridable. I very much like the approach Flag takes in making it so that module authors can lock certain properties from the code. CTools also exports all objects as stdClasses, rather than arrays as Flag does.
I'd be fine with only providing "export" functionality if CTools is installed, but I'd prefer not to introduce a dependency into Flag.
#3
The entire object is overridable, but the 'locked' settings are 100% outside the purview of export.inc; that stuff would easily remain. And is kind of nifty. The implementation is one member variable with a list of locked fields, and then a bunch of #access settings in the UI. export.inc does not care what settings you have. And it's possible to directly get a default even if you have a database object, so you can preserve whatever locks you want.
The array being used seems silly. It is immediately converted to an object -- and slowly, using a PHP loop instead of an (object) cast. That's terrible. I think the reason for the array must be that there isn't an export facility and the array is a little easier to just come up with on your own? But with an export facility, you don't really need that.
I recall discussing with quicksketch about the fact that CTools exports are all named and do not appear in the database at all, and he wanted defaults to appear in the database. I was a little leery about doing that since it required some intelligent database wrangling. It also looks like that didn't make it into the patch.
AS for dependency on CTools, don't:
Then copy export.inc -- if CTools is present it will use the real one, otherwise it will use your copy. You can get the benefits without requiring the extra dependency. Someday in the future if CTools becomes ubiquitous, you can simply phase in the dependency. If it never does, you're in great shape.
The one trick is that the export.inc method intends to own the entire flow, so using it just to export flags but not to control the loading of flags might not work well. It has a facility to load all, load based on conditions, or load an array of IDs, so in theory using it should reduce the size of your codebase and still do everything you want it to do. It also has some settable flags.
#4
Loading might look something like this?
<?php
/**
* Load a single flag.
*/
function flag_load($name) {
ctools_include('export');
$result = ctools_export_load_object('flags', 'names', array($name));
if (isset($result[$name])) {
// I'm rusty on my bitwise math so I'm guessing. I get this wrong the first time a lot.
if ($result[$name]->export_type & (EXPORT_IN_DATABASE + EXPORT_IN_CODE)) {
$default = ctools_get_default_object('flags', $name);
// ... go through $default->locked and set properties. This ensures that code updates
// will always have their settings immediately apply.
// optionally check for changes and if necessary write them back to the database so
// that queries don't fail on the wrong information.
}
return $result[$name];
}
}
?>
#5
in addition to the exporter for default flags it would be nifty if one could just import them again...
#6
subscribe
#7
This feature request stands out as one more important ones.
Views has import/export as well as flag integration, so this feature is necessary to provide consistency in contrib.
#8
It's not critical. I updated the handbook page a few days ago with docs on how to make a default flag from within a module: http://drupal.org/node/305086
#9
Currently what would the best way of getting the array/object that represents a flag to use in the default hook? Just loading one up and printing it out? Any other good way?
#10
Yep, printing it out and then converting it to an array is a good approach. I've just been hand-typing them based on the sample in the handbook.
#11
Moving
#12
subscribing
#14
Based on http://www.stellapower.net/content/using-ctools-module-create-exportables I've created a patch that allows importing/ exporting flags.
TODO:
1) Create an upgrade path - currently in order to test it you need to reinstall module.
2) Add Ctool's export.inc to flag, so there will be no dependency.
I'm marking as needs work, although I'd appreciate somebody with more Ctools experience to have a look :)
#15
1) I've added a loading wildcard
%flagfor the menu system, so/admin/build/flag/WRONG-FLAG/exportwill result in a page not found.2) Haven't added dependency, but added a message about CTools needed to be installed for import/ export.
3) Added message about using features to pack code.
4) Used hook_schema_alter() to add the 'export' information to flag's schema -- although I wonder if there's a better way.
#16
I get WSOD when trying to enable feature with a flag, so back to needs work.
#17
Patch fixes fatal error.
However there is still an issue with Features using CTools to export #552496: Allow modules to add additional data in ctools_export_object()
#18
edit: commented the wrong issue. Moved -> #490462: List of Modules that would need export/import stuff.
#19
@fago,
Would you suggest to neglect the CTools idea, and just provide own implementation?
#20
Removed dependency in CTools. Flag is now able to import/ export and is integrated with Features.
#21
Subscribing. Is this patch safe to use in production environments?
#22
@EvanDonovan,
Not yet ready for production.
#23
Flag is now passing a pipe with the flagged type, so Features can retrieve dependencies -- in other words, also you content types and modules are added to the feature.
#24
The patch in #23 failed for me. I tried against 1.x-dev and 2.x-dev to see if it was mismarked.
#25
@q0rban,
Can you explain what failed?
#26
Ok, I saw the problem - re-rolled.
#27
Generally I think adding the import/export functionality to Flag is a great idea. However, I have huge reservations about making this functionality dependent on CTools. What's so hard about doing a var_dump() then doing an exec() to read it back in? Features also isn't required to export to a module, since Flag provides hook_flag_default_flags() without Features just fine. Even though Features isn't required to do import/export, general support for it sounds fine with me, but it shouldn't be required in any way (and preferably kept separate from flag.export.inc).
I'm not sure if CTools is required or not from reading the code. It doesn't look like it's used anywhere, but then there's this line of code preventing import:
+ if (!module_exists('ctools')) {+ $form['ctools'] = array('#value' => t('You must install the <a href="@ctools-drupal-project">CTools</a> module in order to import a flag.', array('@ctools-drupal-project' => url('http://drupal.org/project/ctools'))));
+ }
Other than my general opposition to requiring CTools/Features, I'm okay with most of the code here. However I'd like to see this unbound from these two modules and then have support added for them separately.
#28
Note that the implementation in #26 doesn't depend at all on CTools -- The CTools part is just cruft from previous patches, that I should remove, and re-roll.
Same thing goes with featuers. The import/ export doesn't depend, it just implement Feature's hooks.
#29
This sounds great. We should put the Feature hooks (other than hook_features_api()) into flag.features.inc rather than flag.export.inc.
#30
Ok, I hope I re-rolled it properly, it's late... ;)
#31
Thanks Amitaibu, looks much better! Not a mention of CTools in the whole thing. :D
I'm interested in the way you've made $flag->validate() return TRUE and FALSE for validation success (a good idea). I did something similar in #285237: Ability to disallow a flag/unflag operation, where instead of calling form_set_error() at all, I'm now returning a list of errors to flag_form_validate() and letting it call form_set_error(). This means that rather than just telling the user "Import failed", we could tell the the reason for the failure. Thinking about it, this patch probably does this already but is probably using form_set_error() improperly, since the field that is receiving the error isn't even on the import form.
Anyway, I'll split out that change into a separate patch so neither this patch nor my other one needs to include it. I'll reroll this patch while I'm at it. Thanks for the patches so far!
#32
I committed #589562: $flag->validate() should return a list of validation errors, which should trim up a few changes in this patch. I'll reroll and post shortly.
#33
I updated the patch to accommodate for the latest changes, and made the following adjustments:
- I moved all export functionality to flag.export.inc.
- I moved the path of the export to "admin/build/flags/export/[flag-name]" to match the other Flag admin paths. It also prevents namespace collisions that would occur if a flag were named "add", "edit", or "list".
- I made "admin/build/flags/export" a stand-alone form to select a list of flags to export, similar the approach provided by views_export.module.
- Code style and comment cleanup.
#34
On a reading review I noticed a minor issue:
+ 'flags_edit' => array('title' => t('edit'), 'href' => 'admin/build/flags/edit/' . $flag->name),+ 'flags_delete' => array('title' => t('delete'), 'href' => 'admin/build/flags/delete/' . $flag->name),
+ 'flags_export' => array('title' => t('export'), 'href' => 'admin/build/flags/export/' . $flag->name),
D6 style should be
'edit/'. $flag...#35
The 2.x version of the module is using the 7.x style for string concatenation, since we'll be maintaining the 2.x version in both D6 and D7 simultaneously. This makes applying patches between the two versions much easier. Oddly mooffie and I were using different syntax in the 1.x version of Flag (I was using a space on one side, he was using a space on both sides), but in the 2.x I'm working on standardizing on the new format as we go along. So the change is intentional and we'll be using it from now on.
#36
- Re-roll so it will apply properly.
- include export.inc on flag_features_export_render()
- Load flag from flag name on flag_export_flags() - as features passes the flag name, not the flag object.
#37
This is the same patch as #36, however it's without the piping of the content types related to the flag. I think it's better to allow the user to define the content types, as another feature might already be incharge of creating the content type.
#38
please please PLEASE GET THIS IN!
#39
and, i think it'd be better to pipe the content types, because we're not always in the ideal situation where someone else defined the content types. If you reference a non-existent content type, well, i don't wnat to know what happens (but let's assume the world implodes)
#40
>please please PLEASE GET THIS IN!
AFAIK quickskecth plans to have it for the 2.x release.
> i think it'd be better to pipe the content types
The reasons I think it should not pipe are:
1) Maybe someone else already exported the content type to another feature.
2) The CCK handling of features is still lacking (e.g. no group fields support). I for example use http://drupal.org/project/cck_sync for now (and will kill the module when Features will do a better job).
3) Allowing the user to add/ remove dependencies is maybe less sophisticated, but more flexible in this case.
#41
I reviewed this again and everything is working great. I downloaded Features and tried it out for the first time. I'm not entirely sure I "get" it, but I see what it's trying to accomplish and how it works. Regardless, the Flag implementation seemed to work flawlessly, as did the direct import/export support.
Committed to the 2.x branch. Thanks Amitaibu (once again) for your excellent work!
#42
Awesome, thanks! :)
#43
oh, maybe worth mentioning this feature in the front page.
#44
I'll add a list of new features in the 2.x version when we publish the first beta release. Right now I don't want to explicitly encourage using the 2.x version though, as it's impossible to downgrade back to the 1.x version if you have troubles.
#45
Yeah, you are right - I was probably just excited about it being committed ;)
#46
Automatically closed -- issue fixed for 2 weeks with no activity.
#47
@quicksketch,
you can add the "Features integration" on "Version 2.0 Information"
#48
Any hope of getting flag_actions support as well?