Adapt indexes and servers to new core Entity API [#2044087]

The Entity API module is now in core, and was changed a lot. From what I currently understand, entity types are now plugins and discern between content entities and config entities. There is some documentation available, though I can't really find a canonical change record. I'll have to see how complete (and helpful) the documentation is and whether I can make sense of it.

It seems clear, however, that both types should be config entities, so this seems like a good change to me.

Comment	File	Size	Author
#7	search_api-start-converting-server-entity-type-2044087-7.patch	11.46 KB	freblasty
#7
#3	search_api-start-converting-server-entity-type-2044087-3.patch	21.1 KB	jsacksick
#3
#3	interdiff-2044087-2-3.txt	462 bytes	jsacksick
#2	search_api-start-converting-server-entity-type-2044087-2.patch	21.11 KB	jsacksick
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

drunken monkey

he/him

German

Vienna, Austria

CreditAttribution: drunken monkey commented 21 July 2013 at 21:28

I'm currently in the process of updating the index entity. A bit easier, since it doesn't involve service class plugins (which I'm thinking about renaming "server plugin", taking blocks as inspiration/orientation). Still not trivial, though, and a lot of work.

One thing we should figure out, after the handiwork of this update step is done, is whether all the helper methods (indexItems(), reindex(), etc.) should be directly on the entity object, or whether we should separate them into a different class. It seems to me, entity classes aren't usually that overloaded with additional logic, but a few methods are regularly added, so I'm not sure what's the best practice here.

Another question is whether we should rename the properties to more common names – i.e., id instead of machine_name, label instead of name and maybe status instead of enabled. I think that my unusual choice for the property names has caused some confusion in the past, and since we are already moving the entities from the database to the config, renaming shouldn't really be any problem. Only downsides would be the necessary additional code changes and that people already familiar with our specific naming would have to forget that again – or, rather, balance both in their heads until they're not using D7 anymore.

Comment #2

jsacksick CreditAttribution: jsacksick commented 9 August 2013 at 15:04

Status:

Active

» Needs review

File	Size
search_api-start-converting-server-entity-type-2044087-2.patch	21.11 KB

Here's a first pass, there's a lot to do and it's complicated to process everything at once, it would be nice if we could commit something quickly that could unblock other developments.
This patch mostly touch the Server entity type, and also remove some unneeded code such as hook_entity_info(), hook_entity_property_info().

Comment #3

jsacksick CreditAttribution: jsacksick commented 9 August 2013 at 15:08

File	Size
interdiff-2044087-2-3.txt	462 bytes
search_api-start-converting-server-entity-type-2044087-3.patch	21.1 KB

Comment #4

drunken monkey

he/him

German

Vienna, Austria

CreditAttribution: drunken monkey commented 13 August 2013 at 23:25

Status:

Needs review

» Active

Thanks a lot for your help here, looks great! I've committed it right away, so to make it easier to work off it.
I'd offer to give you temporary commit access (once again), but I fear that would make it too hard for me to keep track, and also increase the danger of conflicts. So I'd prefer to keep using patches for now, unless you think commit access would make your life that much easier. I really appreaciate your help, so I'll of course try to support you as best as I can.

Regarding the patch, I have a few notes, but those are as much to remind me later as for you. So, if you don't go back and fix them, I'll just do it when I resume my work on the port.

```
@@ -7,15 +7,38 @@
+ *     "id" = "id",
```
I think we should keep using machine_name as the ID field, like we did for indexes.
Or at least do the same thing for both.
```
@@ -184,7 +220,7 @@ class Server extends ConfigEntityBase {
-  public function postCreate() {
+  public function postCreate(EntityStorageControllerInterface $storage_controller) {
```
We can't just do this, postCreate() has a different meaning in D8 than it had in the Search API in D7. We used it to react to an insert, while this method will now be called as soon as the object is created – even if it is never saved.
So, this should be moved to postSave() with appropriate checks.
```
@@ -0,0 +1,26 @@
+class ServerAccessController extends EntityAccessController {
```
Hm, if the access controllers both stay like this, maybe we could use the same for both entities? Or is that discouraged/a bad idea?

@@ -1822,6 +1630,8 @@ function _search_api_extract_entity_value(EntityMetadataWrapper $wrapper, $fullt
+ * @deprecated use entity_load('search_api_server', $id) instead.

Why should this be deprecated?
Also, please see the correct order of Doxygen tags.

Comment #5

drunken monkey

he/him

German

Vienna, Austria

CreditAttribution: drunken monkey commented 2 September 2013 at 15:13

As per #2044421-5: [meta] Upgrade to Drupal 8, I've moved the entities to the Drupal\search_api\Entity namespace.
Now tackling the above notes.

Comment #6

drunken monkey

he/him

German

Vienna, Austria

CreditAttribution: drunken monkey commented 2 September 2013 at 16:40

Fixed the notes from #4.

Comment #7

freblasty CreditAttribution: freblasty commented 5 September 2013 at 21:56

Status:

Active

» Needs review

File	Size
search_api-start-converting-server-entity-type-2044087-7.patch	11.46 KB

Fix for correct usage of entity_load() (blocks #2044093: Adapt menu entries to new Routing system).

However in Drupal 8 a lot of the functionality has moved to controllers which support the use of dependency injection. Therefor I would opt for some kind of decorator around the entity storage. This way it can be declared as a service and the functions in search_api.module can rely on corresponding managers until they get removed.

Example:


  class ServerManager {
    public function load(...) { }
    public function loadMultiple(...) { }
    public function loadMultipleByProperties(...) { }
  }

  class IndexManager {
    public function load(...) { }
    public function loadMultiple(...) { }
    public function loadMultipleByProperties(...) { }
  }

  // Example usage in search_api.module
  function search_api_server_load(...) {
    return Drupal::service('search_api.server.manager')->load(...);
  }

Any thoughts?

EDIT: Also all helper functions that are now declared in search_api.module would then move to the corresponding manager.

Comment #8

drunken monkey

he/him

German

Vienna, Austria

CreditAttribution: drunken monkey commented 5 September 2013 at 22:10

Status:

Needs review

» Active

Thanks a lot for the patch! Committed.
I also removed my custom Server::getIndexes() method for doing exactly that – I didn't even know entity_load_multiple_by_properties() existed, that's of course a far better alternative.

Regarding your other suggestions: What's core doing in that respect? If you think it makes sense and there's a few places where core is doing it (or plans to do it), why not?
Even though, right now, I don't really see any benefit here. After all, the entity_load*() functions use a DI container method internally anyways.
So, if you could explain to me why we should do that, it would be great!

And one other thing, regarding your patch:

diff --git a/contrib/search_api_facetapi/search_api_facetapi.module b/contrib/search_api_facetapi/search_api_facetapi.module
old mode 100644
new mode 100755

Next time, please take care not to include such file mode changes in patches!

Comment #9

drunken monkey

he/him

German

Vienna, Austria

CreditAttribution: drunken monkey commented 5 September 2013 at 22:11

(Regarding the status, I don't even know whether this isn't already fixed now. It seems to me like the basic entity framework is now in place, only some method bodies need to be updated.)

Comment #10

freblasty CreditAttribution: freblasty commented 6 September 2013 at 00:34

Must have forgotten to execute git config core.filemode false when I cloned the repository. Sorry about that!

Comment #11

freblasty CreditAttribution: freblasty commented 7 September 2013 at 01:25

Regarding your other suggestions: What's core doing in that respect? If you think it makes sense and there's a few places where core is doing it (or plans to do it), why not?
Even though, right now, I don't really see any benefit here. After all, the entity_load*() functions use a DI container method internally anyways.
So, if you could explain to me why we should do that, it would be great!

The bigger picture here is to remove helper functions from the module file and place them in corresponding manager, controller or helper classes which can be used by the DI container.

An example is Drupal\system\SystemManager which provides the method getAdminBlock() as oppose the Drupal 7 variant system_admin_menu_block(). This also provides a better test coverage as with DI you can replace the parts with stubs during tests and removes the hard-wired links.

I'm guessing the main reason that helper functions like entity_load() still exist is because some parts of core haven't been fully converted yet and therefore require Drupal 7 style functions.

Comment #12

drunken monkey

he/him

German

Vienna, Austria

CreditAttribution: drunken monkey commented 7 September 2013 at 10:29

Hm, OK. So you'd say even entity_load() might get removed?

Would you say doing this the right way right now would save much time compared to just making things work and polishing later? Otherwise, I'd prefer getting things working first and than polishing them according to core standards during the Beta phase. This would also have the advantage of being better able to tell what the core standards actually are.

But if you'd rather use managers right away, and are confident that that's the way to do it in Drupal 8, then please go ahead and do that. It does make sense, you're right, and in general I'm all for OOP. (The half-assed use of it was one of the things that annoyed me most in Drupal 7.)

Comment #13

aspilicious CreditAttribution: aspilicious commented 10 September 2013 at 19:28

entity_load will most likely stay because it does more than solely loading of the entity. But if you look at the implementation you'll see it is just calling load on the storage controller. So if search api has custom load logic it needs to implement its own load function in a storage controller.

So you don't need to write a custom class that wraps the loading functions as described above. On the other hand you should use the 'entity manager service'.
If you're extending the controller base class you already have the entity manager function provided. So your code should look like this for example:

$this->entityManager()->getStorageController('server')->loadMultiple(...)

In procedural code you can use:

Drupal::entityManager()->getStorageController('server')->loadMultiple(...)

Or use entity_load / entity_load_multiple for the time being :)

Comment #14

freblasty CreditAttribution: freblasty commented 11 September 2013 at 00:31

The manager classes noted in #7 are not to replace or enhance the storage controller but simply more of a helper class which contains all the helper methods that normally would reside in the module file. That way DI can be used on the helper classes and the dependency with the module is file is no longer required or hard-coded.

@aspilicious: So you're saying for Drupal 8 some parts of core will ship with mixed support, procedural and OO (I'm not talking about hooks)?

Comment #15

aspilicious CreditAttribution: aspilicious commented 13 September 2013 at 16:27

Your "helper" functions (load, loadmultiple, ...) are already part of the entityManager which is injectable. You only should write your own injectable class if it's providing MORE than just wrappers around entitymanager()->getstoragecontroller()->load()

If the code in the module file is solely about fetching/saving stuff than it's maybe part of the customized server/index storage controller... :)

And yes procedural and oop will be mixed for drupal 8. Lots of code will be part of classes, some stuff will stay procedural.
We can't do much about it.

What is going to stay procedural and whats going to be oop is THA question I can't answer today. :)
Drupal 8 is a moving target...

For example I don't think 'drupal_render' is going to turned into an OO alternative in this release cycle.