The entity API provides basic features integration that should basically work. I just gave it a test, the export of the index looks rather good, but I doubt the export of the server can work:

/**
 * Implementation of hook_default_search_api_server().
 */
function recruiter_job_search_default_search_api_server() {
  $items = array(
    'default_db_server' => entity_create('search_api_server', array(
      'module' => 'search_api',
    )),
  );
  return $items;
}

I do not know why, but why aren't there any properties? I doubt there is nothing saved? At least the service class should be there?
If necessary, you can override the controllers or the class' export() method.

Additionally, but not critical you could override the feature integration defaults to add in the index in use of a server automatically. But that's another story.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey’s picture

Title: features integration troubles » Features integration troubles
Status: Active » Needs review

Oh, yeah, that's one of the other drawbacks of using __get() for the properties: they won't be listed anymore with a normal foreach. Nor with anything else I know of, for that matter.
I just overrode export() now. Seemed straightforward enough. It should work now, please try it.

fago’s picture

Title: Features integration troubles » Fix exportables
Priority: Normal » Critical
FileSize
19.67 KB

I had a look at, however I noticed that this is far not the only problem as the code doesn't make much use of the entity CRUD API at all? Instead it seems to re-invent the stuff again :(

That's bad, not only as it's more code to maintain and fix and it isn't consistent with the other entities, but what's worse the exportable stuff doesn't work..

So here is a patch that fixes the code to properly use entity_load() and makes use of machine names as id, which is necessary to don't break up the reference upon export.
The updates could certainly need a cleanup to use the in place $index->save() stuff as well as the forms. But that's another story :/

fago’s picture

FileSize
18.16 KB

seems of your recent changes got mixed in the patch. fixed the patch.

fago’s picture

grml, next try

drunken monkey’s picture

Priority: Critical » Major
Status: Needs review » Needs work

Wow, that's quite a patch, thanks! There sure are some useful parts to it, but I also have a few problems in it.
Apart from a few places were you missed an "id" or such, the main problem is that for my taste you change far too much here, much more than I think would be necessary. However, I still don't really get the whole exportable stuff, so maybe you can explain again what the real issue is here. Or, I'll start with what I understand and you correct me:
When exporting, the IDs might change, so storing the index' server as its machine name instead of the ID makes sense. This of course also necessitates changing all places where $index->server is used, one way or another.
Then, what? Exportable entities can be stored in code, making normal database queries pretty much useless for dealing with them? I really don't understand why someone would consider that a good idea, and not just save the exported entity in the database when it woul be returned the first time. But if this is really the case, a lot more places would have to be updated, as only SELECTs on the search_api_item table would then still be reliable.
But even if the latter is also true, exportable entites will still have IDs, which are locally unique, right? So why change "id" to "machine_name" in all those other places, where it is only used internally inside a single function? And why change the paths to use the names everywhere – I agree that it can be deemed more aesthetical this way, but it isn't really necessary, right?
And again, if there is a reason that IDs shouldn't even be used internally, then a whole lot more places would need to be changed.

Also a few other questions:

+++ search_api.module
@@ -234,7 +234,6 @@ function search_api_entity_info() {
-    'load hook' => 'search_api_server_load',

Why remove those? Will they still be called without this definition?

+++ search_api.module
@@ -1045,7 +1038,7 @@ function search_api_server_insert(array $values) {
-  $server = search_api_server_load($id, TRUE);
+  $server = search_api_server_load($id);

Why change this, what has it to do with this issue?

I had a look at, however I noticed that this is far not the only problem as the code doesn't make much use of the entity CRUD API at all? Instead it seems to re-invent the stuff again :(

I didn't really find any documentation on how to use that functionality, what is provided, etc., so opted for the easier way of implementing it myself. Maybe I'll change those parts some time later.

Powered by Dreditor.

fago’s picture

I didn't really find any documentation on how to use that functionality, what is provided, etc., so opted for the easier way of implementing it myself. Maybe I'll change those parts some time later.

Well, as the name says, it's an CRUD API. So if you don't use it for CRUD, it's not used correctly. E.g. API functions ala entity_save() won't properly save you have not adapted the save callback, but wrote your own. So either use it, or not.

@exportables:
I think you got them right in general. So yes, you may not do queries as entities may live in code. Still you might need some separate tables even for entities in code (like your items). entity_load() should be the only way you get exportables.

>I really don't understand why someone would consider that a good idea, and not just save the exported entity in the database when it woul be returned the first time.

I thought about this, but this poses some troubles, like including translated strings or ensuring updates work properly. So as of now, this is how exportables work in drupal (for ctools too).

@numeric-ids:
Mostly they are there, because the field API and so the core entity API requires it. So I'd suggest to mostly use the machine name, expect for storing the entity in the database. Still you can use the numeric id if you prefer, but always be aware that for entities in code, there won't be an numeric id. So for being able to refer to exported entities, you'll need the machine name.

So to fix everything, it should be fine to just change the previous numeric id to the machine name and ensure storage works. Having in machine names in URLs too is nicer yes, and required for being able to view exported entities.

@- 'load hook' => 'search_api_server_load',
Yep, that's default.

@- $server = search_api_server_load($id, TRUE);
Sry, that's indeed unrelated. It just didn't make any sense to me always clear the cache. But right, this should be another issue.

drunken monkey’s picture

I think you got them right in general. So yes, you may not do queries as entities may live in code. Still you might need some separate tables even for entities in code (like your items). entity_load() should be the only way you get exportables.
[…]
always be aware that for entities in code, there won't be an numeric id.

Oh, wow, that sucks. :-/ I'd definitely consider this a drawback to address in D8 …
But as it is, it really seems like I'll have to rewrite half of the base module. This might take a few days … (At least I can include #940348: Fix to use CRUD functions of the entity API as well, while I'm at it.)

Sry, that's indeed unrelated. It just didn't make any sense to me always clear the cache. But right, this should be another issue.

The function loads the entity to check if values were changed. If it loads the cached entity that the user might have used to set exactly those new values, it won't get updated.
Of course, core just does an update nonetheless in such cases, whether any values changed or not – but I didn't know that back then and thought this should be basic functionality. Maybe I'll drop it during the re-write.

drunken monkey’s picture

By the way: If exported entities don't have an ID, then I also have to change the search_api_item table to use the machine name, right?
This will be one hell of a memory explosion …

I think I won't commit this right away, but post it here first for a review.

fago’s picture

Oh, wow, that sucks. :-/ I'd definitely consider this a drawback to address in D8 …
But as it is, it really seems like I'll have to rewrite half of the base module. This might take a few days … (At least I can include #940348: Fix to use CRUD functions of the entity API as well, while I'm at it.)

Ouch. :/ If you need additional conditions supported by entity_load() it's possible to implement that. Just ping me if you need that.

>By the way: If exported entities don't have an ID, then I also have to change the search_api_item table to use the machine name, right?

Right. Indeed the machine name use is probably not ideal here, but I can't think of good workaround here. I'm unsure though how bad it is really in regard to memory.

      'item_id' => array(
        'description' => "The item's entity id (e.g. {node}.nid for nodes).",
        'type' => 'int',
        'unsigned' => TRUE,
        'not null' => TRUE,
      ),
      'index_id' => array(
        'description' => 'The {search_api_index}.id this item belongs to.',
        'type' => 'int',
        'unsigned' => TRUE,
        'not null' => TRUE,
      ),

hm, if you create an index on other exportable entities then there might be no numeric id.. Probably best just exclude exportable entities so they aren't supported? I guess, that's something someone won't need anyway. Exportable entities are configuration, so I guess we won't build searches for them? :)

drunken monkey’s picture

If you need additional conditions supported by entity_load() it's possible to implement that. Just ping me if you need that.

What kind of additional conditions do you mean? I don't think I'll need any, the current functionality should suffice.
The only down-side for my module probably is the missing sort functionality. And, maybe, batch updates, but I'm not sure I used one in such a context.

hm, if you create an index on other exportable entities then there might be no numeric id.. Probably best just exclude exportable entities so they aren't supported? I guess, that's something someone won't need anyway. Exportable entities are configuration, so I guess we won't build searches for them? :)

I thought about that too, but yes, there is hardly a work-around here (without creating a second table for use with exportable types), and I also thought that a use-case for that would be very unlikely.
Additionally, the specification explicitly states that an entity has to define a numeric primary identifier, so exportables are, strictly speaking, not valid entities. Even though it probably makes sense in this case, we can't expect to generically support entities that don't conform to the entity specification.

fago’s picture

Additionally, the specification explicitly states that an entity has to define a numeric primary identifier, so exportables are, strictly speaking, not valid entities. Even though it probably makes sense in this case, we can't expect to generically support entities that don't conform to the entity specification.

Which specification? ;)
The docs only state that there has to be a property for that, and exportables have that. Also un-saved entities have no id yet too, just like exportables defined in code.

drunken monkey’s picture

Which specification? ;)
The docs only state that there has to be a property for that, and exportables have that. Also un-saved entities have no id yet too, just like exportables defined in code.

I don't know, where I originally read it (as we all know, there isn't really any general documentation, like a handbook page, for entities), but the hook_entity_info() documentation contains the following:

id: The name of the property that contains the primary id of the entity. Every entity object passed to the Field API must have this property and its value must be numeric.

So it has to be the primary identifier (which it isn't, for exported entities), and the property has to be set to a numeric value on all objects (not NULL). Unsaved entities are the single exception here and not a wildcard pass for ignoring this in other cases as well.

This is a moot point, since we already agreed that this is an acceptable drawback in this case, but exportables clearly violate this statement. Modules using exportables can therefore not expect that these entities are supported by modules using entities generically. And I think we also agreed on that.

fago’s picture

Indeed, exportable entities override this assumption.

>This is a moot point, since we already agreed that this is an acceptable drawback in this case, but exportables clearly violate this statement.
Agreed.

>Modules using exportables can therefore not expect that these entities are supported by modules using entities generically. And I think we also agreed on that.

Yep, as you noted there is this difference. I doubt though this breaks a lot of entity-related code, but yes it could happen.

drunken monkey’s picture

Something that occurred to me: When an exported entitiy is imported, do I have to do something to build the search_api_item entries for the entity? And, if this is the case, where could I do that?

fago’s picture

As of now, there is no special import hook or such, but the usual 'insert' hook should do it? I don't think importing should be different to the usual creation process then?

drunken monkey’s picture

Well, as of now, this is handled by the save() method, when SAVED_NEW is returned. If this is the case when importing, then that's taken care of, yes.

fago’s picture

yep, should be like that.

drunken monkey’s picture

Almost done …
Just a few questions regarding hook_update_N(): I couldn't really find information on what is safe to use in there and what isn't. I saw in your patch that you didn't use entity_load() – does that mean, it shouldn't be used in those hooks? Does autoload work?

Depending on how much is possible, I have up to two problems, one now and one possibly later:

- If entity_load() doesn't work, how can I, in the future, update exported entities in code? Or should they always be updated in the database version and then exported freshly, anyways?

- In the Solr search module, I want/have to clear all Solr servers when updating, since otherwise the old data will indefinitely remain there (as normal clear() operations would use the index' machine name for filtering, never finding the items indexed with the index ID). Normally, I'd just load all indexes currently lying on Solr servers and do an $index->clear() (with the new functionality) – however, since this would use all kinds of autoload, entity_load and other functionality, I'm not sure if that can be used in the hook.

drunken monkey’s picture

FileSize
129.98 KB

Here is the almost-completed patch. The update stuff isn't tested yet (I'll wait until I know what to do for Solr servers for that), so might break horribly, but on a new install everything seems to work fine.

In fact, while working on this, I found quite a number of unrelated stupid bugs that apparently just went unnoticed until now. So going over all code wasn't such a bad thing after all. ^^"

OK, the patch is about 130k, so you probably won't be reviewing this, but you can test it if you want.

fago’s picture

Status: Needs work » Reviewed & tested by the community

I've not tested it yet, but the code looks good - so it should also fix the CRUD stuff.. :)

The only minor thing I stumbled over was a missing space (if($ret))

+      $this->$field = $value;
+    }
+    $ret = $this->save('edit');
+    if($ret) {
+      if ($reindex) {

So I'd suggest to get this in asap. I'll give it thoroughly test probably tomorrow.

drunken monkey’s picture

Thanks for the note.

What about my questions in #18? As said, it's not completely done yet.

fago’s picture

ops, sry.

>I couldn't really find information on what is safe to use in there and what isn't. I saw in your patch that you didn't use entity_load() – does that mean, it shouldn't be used in those hooks? Does autoload work?

Yep, autoloading works and entity_load() or a wrapper for it should be used wherever you want to get an entity.

>- If entity_load() doesn't work, how can I, in the future, update exported entities in code? Or should they always be updated in the database version and then exported freshly, anyways?

You can just edit + update (->save()) exported entities as usual. Re-exporting them is up to the users. Probably it would be nice to show the exportable status to the user. There is an entity theme function for that. Also 'delete' should be called 'revert' then. Maybe have a look at the admin UI of profile2 as example (which is driven by the just added generic entity UI). But I'd suggest to such UI optimizations in a follow-up - you could build upon the entity UI too, but it's probably too late for that as you already have a nice UI.

>In the Solr search module
You mean for the upgrade of the module now? Hm, you shouldn't use any of your API functions in your update functions as they may not be there/work a bit different at a later point. So best directly query the db for servers + clear them with low-level APIs.
Generally having stuff autoload during _update() should be fine though.

drunken monkey’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
131.47 KB

OK, it's done now and exhaustively tested (I hope).

fago’s picture

Status: Needs review » Reviewed & tested by the community

Great, looks good to me. :)

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

OK, I just committed it. Thanks for your reviews and other help in here!

This is a list of (hopefully) all API changes in the patch:

  • (Nearly) All usages of server or index IDs were replaced with the machine names.
  • Hence, schema changes to {search_api_index}.server and {search_api_item}.index_id (same for facets and pages).
  • Hence, also, changed paths for both (%search_api_server and %search_api_index are now the machine names) – although the old paths would still work, you shouldn't use them, for consistency's sake.
  • The Service::removeIndex() method changed, too, in now receiving the index' machine name instead of ID when an index is deleted.
  • Added/overridden CRUD methods in SearchApiServer and SearchApiIndex (which should be used instead of the search_api_ENTITY_OPERATION() functions – although the latter will work as before).
  • search_api_list_server() and search_api_list_index() are now deprecated.
  • Also, some form internals changed – only relevant if someone already does a hook_form_alter() on one of them.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.