Make facets exportable

Yes, probably … *subscribe*

Does the attached patch fix this?

As I just introduced the numeric "id" field, there won't be any places where I have to replace the numeric ID with the machine name this time, but there may be other places with implicit assumptions that I haven't found.

Note to self: Before committing I should replace all search_api_facet_save() calls with the OOP equivalent. Won't have any impact on correctness, though. Some search_api_facet_delete() calls might also be replacable.

Re-rolled because of the recent commits in the module.

@search_api_facet_load()
entity_load() really should work the same way as your function, but with your code entity_load() won't be able to deal with the status. To fix that, you'll need to override the controller's load function and do your $condition kung-fu there.

Oh no, controller-class-overrides, my mortal nemesis!
But why would it matter if search_api_facet_load() doesn't work like entity_load()? Code uses one or the other anyways, doesn't it? Otherwise I'd just create another function that does the block status query part, which would (in my opinion) not make a difference.

Also I'd not suggest over-ruling the semantics of the argument based upon what is passed. That's considered bad style and makes things confusing.

Yes, I know that, it's ugly. I'll fix it soon. Just didn't want to break existing API, but of course it's very unlikely anyone already uses those functions, anyways.
Creating a whole module more or less on a quick-and-dirty basis (OK, not really, but not as clean and shiny as the other parts) just had to make problems some day …

But when I create an additional search_api_facet_load_multiple() function, it would get the whole block status magic thingie anyways, so search_api_facet_load() would be "pure" again – would that be enough, or are there also issues with the search_api_facet_load_multiple() function containing voodoo?

Again, just using entity_delete() has to be equivalent. Override the controller's delete method to apply your additions.

:(
Or, I could implement hook_~_delete() …

This won't work due to the static-caching of entity_load().

Really, even with $reset = TRUE? I don't think so, but wouldn't be too surprised being proven wrong. However, as you said, the now possible alternative is superior anyways:

Though you can use $entity->orignal, once I've fixed #990558: Allow reacting on changes.

Awesome, finally! This means I can get rid of the whole WTF-code with $fields in my hooks, etc., right? Seems like it, great! :D

Oh, and that would also make #964816: Provide own EntityController for fixing hook invocations unnecessary — YES! :D

Again, just using entity_delete() has to be equivalent. Override the controller's delete method to apply your additions.

Didn't spot this right away, but: As far as I can see, entity_delete() calls $entity->delete() anyways (if available), so the two are inherently equivalent, aren't they?
Moving the code into the controller class or into a hook would remove the possibility to delete a facet without clearing the caches (which makes sense for multi-deletes – although it probably wouldn't be too bad of a performance worsening otherwise).

Updated patch that should address all your comments, apart from the above.
Also, I'll update the whole module for #990558: Allow reacting on changes after this issue is resolved, so the patch doesn't yet contain those changes.

Updated once more, removing the SearchApiFacet::delete() method override and using entity_delete_multiple() instead in search_api_facet_delete_multiple().

By the way, just discovered hook_entity_enabled/disabled() — awesome! Tell me such things next time, I can't just read all commit messages … ;)
So I can also finally get rid of the manual hook_modules_enabled() hack, great!
(Hm, searched for linking, but didn't find anything: is there no issue for this addition?)

Also, is it documented anywhere that your modules also creates hook_TYPE_HOOK() for all entity hooks? This is rather important, but neither mentioned in the .api.php nor in the README.txt.

@ fago: Please see also #991632-1: Incorporate newly available entity hooks.

I agree that the delta-generation is a bit of a weak point. However, I can't think of a good solution, either (letting users choose the deltas would be just too bothersome for them). And users running into this problem can always change the default entities' machine names. Or clone the database facet and then reset the "overridden" old one.

Regarding UI: Why shouldn't you be able to override and disable a default entity? Only possible improvements I could readily think of would be to add a "Status" column listing the facets' entity statuses, showing something like "Empty" (or just nothing) for yet undefined facets; and to not display "delete" checkboxes for default entities. Also good would be to somehow specify that checking "delete" for an overridden facet will just reset it, but since the checkboxes aren't individually labelled, I can't think of a good way to do so. (Or, just display an asterisk next to them and somewhere on that page explain what this means?)

Committed the main chunk, but leaving the issue open for the UI stuff.

Hm, indeed. I just thought "enabled" equals "there is a facet entity", but that's probably not the case?

No, there can also be entities for disabled facets, if they were enabled at some point in the past. Otherwise, the "delete" option wouldn't be necessary.

The status column sounds good, then instead of "delete" I think the text should just say "Revert to to defaults" or similar.

As said, there isn't a label for each individual facet, so we can't just write "Revert to the defaults" for overridden ones. But I think the asterisk is a good compromise, I'll do that (along with the Status column).

Updated UI (and cleaned up saving process).

Would it be better like this, with "New" instead of "Undefined"?

Sounds reasonable, too. If no one else comments, I'll do this on the weekend.

Done.

OK, and now it should really work — up to now, the table would be broken when the status was displayed.