Integration with features module?

subscribe and +1, as this would be a great addition to make features work with taxonomy etc.

Hello!

I've been doing some work on this lately, and I thought I'd attach a first-draft patch for you to review…this patch provides all the features API stuff for taxonomy vocabularies; I've tested it out and it seems to work OK, but there are a few potential problems that will need resolving:

1. Features recommends that exported components have machine-readable names rather than sequential IDs; you've done a great job of adding this functionality to vocabularies…however, since this is third-party add-on functionality and not part of the core taxonomy module, other kinds of components don't expect it to be there. For instance, if you export a view that filters on a certain vocabulary, the resulting code will define the filter in terms of the vocabulary's numeric ID, not its exportables-provided machine-readable name. If you enable that feature on a site that already has a few vocabularies defined, the vocabulary ID expected by the view might not be the ID the vocabulary was actually assigned.
2. I noticed one unusual behavior that I may need to spend some more time trying to reproduce…the original imported vocabularies didn't have their node-type assignments properly created, so I deleted them and tried to get features to revert to the coded version…at that point it actually ended up creating the vocabularies multiple times, and the only way I could fix it was by clearing out the vocabulary table entirely. Dunno if it's a bug in my features integration or exportables_sync(), but it'll take some analysis (I'm going to look at it some more myself later, but I wanted to get the ball rolling on this patch first.)

I also noticed that there haven't been a lot of recent commits to this project; just wanted to say I'd be happy to help if you'd like a co-maintainer. I'd also love to see some of this stuff integrated into the features module itself, at least the functions in exportables.module…but that would of course take some buy-in from the features folks themselves.

Anyway, give the patch a whirl and let me know what you think; I'm going to keep trying to improve it over the next few days myself.

Thanks!
Adam

Forgot to change the status…

Thanks for the patch! It would be great if we could roll the features integration into the main exportables module, not the taxonomy part, so the implementing modules wouldn't have more code to write

Hello!

I've updated the patch such that a lot more of the features integration can be handled by component-agnostic code in exportables.module. However, due to the way the features module seems to work, implementing modules will still need to implement each of the features API hooks themselves, just wrapping around the versions provided by exportables (see the last two functions in taxonomy.inc for an example).

Beyond that, the $pipe system in hook_features_export() will often necessitate that implementing modules write some additional code in their export hook so that dependency autodetection works correctly; again, see my example. The downside with how I've set it up in this version is that each vocabulary gets loaded from the database twice during export, but implementing modules can avoid that if they prefer by just not using _exportables_features_export().

Anyway, hope this helps; let me know what you think! I'm pretty highly motivated to work on this right now, so the more feedback, the better :)

Thanks!
Adam

[Edit:] The extra comments above the export_render() hook can probably be removed…I was kind of just taking notes for my own benefit at that point :)

One other thing worth mentioning…the views integration issue I mentioned earlier (comment #2, point #1) is actually relevant to stuff that's already in the features module too, so this may not be the best place to solve that particular problem. I created a separate issue here: http://drupal.org/node/592444

That still leaves the strange duplication problem (comment #2, point #2), which I have yet to explore further…hopefully I'll get a chance to test that out again later.

Thanks!
Adam

Hello!

Got another updated version of the patch, mainly just for the sake of responsible API documentation :) I also updated a couple of function calls on realizing you'd provided a function for doing something I had done a different way.

Once I get back at this on Monday, I think I have a solid plan for fixing that duplication issue I mentioned; just haven't had the time to finish it this week.

Thanks!
Adam

Patch has been committed, and should appear in the development release next time it's generated.

I'm pretty sure the duplication issue is related to the way we're syncing content with the database; I'm going to post another issue about that shortly so that we can track the problem separately. Given the current syncing approach, however, all the features integration stuff is in place and ready to test…not production-ready yet, but certainly ready enough to try out.

Thanks!
Adam

Ah, picked the wrong status; this really should be "fixed", so that any issues with it can still be easily raised.

The issue isn't fixed, it needs more (technical) review.
Here are some comments on code style (can't test functionallity atm).

_{This review is powered by Dreditor.}

+++ exportables.module	2 Oct 2009 21:23:01 -0000
@@ -292,3 +292,85 @@ function exportables_machine_object_prop
+

Unnecessary empty line.

+++ exportables.module	2 Oct 2009 21:23:01 -0000
@@ -292,3 +292,85 @@ function exportables_machine_object_prop
+ * @param string $type name of the exportable, as in the array keys returned by hook_exportables()
+ * @param array $data the data array passed to hook_features_export()
+ * @param array $export the export array passed to hook_features_export()
+ * @param string $module_name the module_name passed to hook_features_export()
+ * @return array same return value expected by hook_features_export()

Comments needs to be full sentences.

+++ exportables.module	2 Oct 2009 21:23:01 -0000
@@ -292,3 +292,85 @@ function exportables_machine_object_prop
+    } else {

Code style: new line for else {

Uhm, sorry for jumping in too late. Didn't notice the code has been commited already. :-/

Code style issues are still valid.

@stBorchert: no problem, thanks for the feedback! I'm still getting used to Drupal's coding style guide, especially the bit about else statements being disconnected from the closing bracket of the preceding "if" …personally I find that a bit harder to read, but I'm happy to adjust to it.

Regarding the docblock comments…actually, I didn't realize that the full-sentence comment guideline applied there, but now that I think of it I'm not sure why it wouldn't. I'll adjust those accordingly.

Out of curiosity, though…I noticed that Drupal is using the Doxygen convention rather than phpDocumentor, which is what I'm used to…do you know of the correct way to specify argument and return value types in Doxygen syntax? The formatting conventions guide doesn't seem to include any examples of this, but in my experience it's extraordinarily helpful for ensuring that you know what's expected and what to expect when using a particular function.

Meanwhile, I can't say I know for sure what the problem is with the unnecessary empty line…in my experience, blank lines can really help improve code readability, as they help the reader to visually chunk things…kind of like the line breaks between paragraphs in this comment. Is that a rule in the coding standard? I don't remember seeing it anywhere.

In any case, I don't mean to be combative…I will be happy to fix these things; I understand how important coding standards are in community projects, and I have every intention of following them. Just want to make sure I do the best possible job of it. Accordingly, I'm changing back to "needs work".

Thanks!
Adam

OK, fixed the docblocks and the else statement. This patch is against the current HEAD; the previous patches had already been incorporated in CVS, so this one's a lot simpler.

Thanks!
Adam

Wonderful :-)

Tipp: using Coder is a real helper while developing.

Committed coding standards fixes this morning.

Thanks!
Adam