Add Views support for individual fields

Following the current trajectory - as approved by dawehner™ ;-) Two sets of fields are added - this has to be in the old hook_views_data:-

Search API results. These have mappings from the datatype to basic fields extending Drupal\views\Plugin\views\field\FieldPluginBase Most of this can be done with the fields in views module (see caveat about the query class below). These are always offered. It's basic, additional formatting, access etc. will require new Field plugins.

Entity results. If the datasource is the site itself, then Drupal\views\Plugin\views\field\Field fields are offered - these will require an entity load. All the entity fields will be available. The formatters, access control etc. are the views entity field ones.
Small bit of background: SQL views fields themselves basically only now work with the Field plugin; which requires the fully loaded entity. Views does this with an entity load (not a set of SQL joins as D7). Thus Search API Entity results can do (much) the same (with the same overhead).

To be clear if you don't add fields from the Entity results list it doesn't load the entity; but you only have the basic field handling.

There are some small changes to the Query class required to make this work. Including at least one dummy method to allow for the fields making SQL specific requests.

Yes it's in a branch. I should get a moment to tidy it up - to actually readable for anyone else - while travelling in the next days and shall push somewhere.

Gentle reminder. ;)

Any update here?

Checking work done - patch attached. I keep only getting to this at sprints, so this is as of Drupalcon, but updated so it should apply to HEAD of now.

Patch adds another view type where the entities are loaded (rather than magically deciding if it needs to load the entities depending on which fields are added - it's just more explicit to the user).

With both types you can:
show the raw search fields from search_api with basic views formatters (note the todo in the code about if these are single or multiple).
With the entity loaded you can also:
access and show the real Drupal entity fields.

Big todo. Patch moves the entity form _item (which was an list) that the rendered entity was using; into _entity (as a single entity) which is where views and fields are expecting it - this breaks rendered entity. So rendered entity needs to look for that.

I've not really looked at the patch very much, but can you tell me why you create base tables not only for each index, but also for each index and each of its datasources? Sure, otherwise it's not possible to add the 'entity type' in the Views definition which might lose some functionality – but having so many base tables seems overkill as a solution for that. Also, it seems it wouldn't be possible with these new handlers to have a search across several datasources?

but can you tell me why you create base tables not only for each index, but also for each index and each of its datasources

Yes it's not ideal, but it was to stop it breaking if there was more than one datasource. From prodding through this views really only supports one entity type, which you need for the fields. You can then have more entity types as relationship entities. It would be easiest to have the indexed item itself as an entity (stored remotely) and it then 'reference', add a relationship, to any local entities; but that's not where we are at. My guess is even when we get to an index with multiple entities it will be difficult to work out a way to include them using referenced entities - but maybe.

And rewriting the fields to get the entity type from the items themselves instead of the definition? Is that not (easily) possible? See also #2601224: Add Views filters, where I use a trait to override the SQL-dependent bits of Views' filter plugins. (But it might easily be the case that this is not easily possible for field handlers.)

Then I'd say the best option would be to have one base table per index, and then specific tables for each entity type (these could, I think, even be shared across indexes, like we currently do in D7?) linked to the base table via relationships. In D7 we already have a similar system for related entities (e.g., tags on the node), the change would be now that the primary items are also already "related entities".
Supporting click sorting properly might need some bit of fiddling, but all in all it sounds like it should be possible.

But of course I haven't really looked into this, so I might talk nonsense. In any case, a pretty complicated issue.

> And rewriting the fields to get the entity type from the items themselves instead of the definition?

This would be rewriting the field display right. The base entity thing is also about the whole field selection, and what is expected where in the result object.

The related entities is probably of more value; but I was struggling to see how to do this without the search api result being the basic entity which then referenced the entities that could be indexed.

Following up f2f:

It might be possible to write a 'relationship handler' with which you could add the indexed entities that want to be loaded and available to use their fields. If possible this would solve having multiple 'base tables' completely: just the raw results array and no _item entity, and then requested _reationship entities.

> It might be possible to write a 'relationship handler'

It is.

Just checking work.

It does greatly simplify the base tables, it also makes it explicit which entities (if any) will be loaded. It will require the rendered entity row to change. It requires work (in addition to above notes) because the filter, sort and further relationships of the loaded entities are made 'available' in the views UI - even though they aren't actually. Adding related entities further down the chain for display is almost certainly possible, filters on them makes no sense.

[edit 2015-11-30: I'm now wondering if we'll have to clone all the relationship 'base tables' in views to remove the 'filter' and 'sort' options :(]

A rough merger with the #2601224: Add Views filters which I made to test. Mainly tidies up the mappings in search_api.views.inc using the neater and more thorough version from that patch - not fully checked though.

I made sure this patch (#14) applies to the latest 8.x-1.x. I also added some small codefixes.

Thanks for the re-roll and code style fixes!

I haven't reviewed or tested this at all yet, but by chance saw this at a quick glance:

+++ b/src/Plugin/views/field/StandardList.php
@@ -0,0 +1,33 @@
+ * @ViewsField("standard_list")

The plugin ID needs to be prefixed with search_api.

Would also be good to list this in the list of beta blocker issues, I guess, even if the parent is already declared a beta blocker.

+++ b/src/Plugin/views/query/SearchApiQuery.php
@@ -433,23 +440,113 @@ class SearchApiQuery extends QueryPluginBase {
+    else {
+      // No entity. If it is required remove the row.
+      foreach ($entities_to_load[$entity->getEntityTypeId()] as $join) {
+        if ($relationships[$join['base']]->options['required']) {

I don't understand this at all. Where does $entity come from?

I fixed the issue mentioned in #17.

Still don't understand #18. I added some more documentation in the code but I still don't really understand what's going on.

@Nick_vh: it was defined twice in the file, php doesn't like that.

I fixed 2 exceptions that the tests gave.

@Nick_vh that is a great idea, thanks!

It's a core bug :(

It's not really a bug, more a design flaw of the individual plugins. While it would be, conceptually, pretty easy I think to make all or most plugins backend-agnostic, they are all written specifically for the SQL query plugin, which of course leads to problems for everyone else.

@ Joris: Thanks for your work here! So, what's your first impression? Would you say we mostly just need to fix the remaining test failures (and probably add more tests), or are major architectural changes still necessary? #14 seems to still list some major problems (especially regarding making SQL filters/sorts/etc. available).

@ ekes: In any case, great work, awesome to hear using relationships worked out! Thanks a lot for all your work here!
And yes, it was the same in Drupal 7, the Entity API module had to provide adapted base tables for all entity types due to that problem. This time, at least, it seems we can automate that a lot more, since we just need to remove all non-field handlers, not also change the used field handlers.

Oh, and one more thing I spotted:

+++ b/search_api.views.inc
@@ -6,70 +6,62 @@
+    $table['table']['group'] = t('Index :name', array(':name' => $index->label()));

The :placeholder used here and in a lot of other places is actually just for URLs, everything else should still use @placeholder.

@drunken monkey:

I don't think it'll be as easy as just getting the tests to pass and adding a few new testcases. I still don't really understand everything that's going on in the patch. I hope to understand it better by monday.

Fixes the remark made in #35.

This needed a reroll again. I made a few small documentation changes. I still don't really understand everything in this patch.

go testbot, go.

I started to write a test for this that uses the views UI to change to fields and sets the options trough the UI, but this is still very broken, attaching my work anyway.

This issue is really interesting to learn how Views module works. :)

I found a possible solution while diving into the problem. When Indexing multiple datasources into the same index, we are using the Field Plugin that at some point detects the entity type. This entity type by default is obtained from the base table. Maybe we can change it and add the possibility to obtain the entity type from the field. So trying this possibility I made interesting progress and it works.

The patch only adds base definitions but I'm hardcoding other fields and it works as expected. This is an example. 1 index 2 datasources (NODE + USER)

Formatters are working. Please disable the cache to test.

I'm Adding empty behaviors to show the fields in the image. You can try indexing one datasource and it works but I'm not sure why I needed to reindex a couple of times...

Attached core patch. I think this patch should not break any test. Anyway I need to ping @dawehner to take a look here because I'm not sure if this way is correct. But for the moment it works.

The patch is really simple. I need to refactor to remove methods from the patch #44. But I prefer to review before invest more time in this way.

I was about to test this issue but the search-api part of this no long applies.

Here is a straight re-roll of Joris' patch from #44.

Disregard the above, that completely drops filters support.

Here are both re-rolls, which should now also contain filters.

I also now had a closer look at marthinal's patch in #47. Really does look pretty interesting, if we can use this to simplify our code.
However, I don't quite understand what the big improvement is compared to just using relationships for all our fields? Unless I'm mistaken, it's even possible to add all those relationships to the view by default, so people don't have to manually add them for each search view.
Anyways, would be great to get dawehner's input on this, you're right – did you already ping him?

+++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -214,7 +214,8 @@ protected function getEntityManager() {
+    $entity_type = $this->configuration['entity_type'] ? $this->configuration['entity_type'] : $this->getEntityType();

Why configuration, shouldn't it be definition? As far as I can see, configuration is never used in the whole Views code. There just seems to be a hack in place to merge the plugin configuration (or, what the D8 plugin system thinks is the configuration) into the plugin definition.

Also, why not just override \Drupal\views\Plugin\views\HandlerBase::getEntityType() directly, instead of circumventing some of the calls to it? Seems like this wouldn't do any harm to have available to other handlers, too.

@drunken monkey
This can significantly reduce the code. I need to clean again the patch. We only need to add the fields and change a few lines in the query plugin.
The UI will be the same as in D7.

I agree that we should use $this->getFieldDefinition()->getTargetEntityTypeId() instead of the configuration but not sure about change the handler base.

I'm not sure if the relationships can work as in D7. I mean, for example in D7 if we have a node and you want to add the email of the author, you can do it. By default, we have the possibility to join the user table and obtain the fields. But with the patch #44 I'm not sure if it works. with #47 I'm trying to understand if I can do the same but ... with multiple datasources... to be honest I need to invest more time to understand how to do it and if this is possible.

I think we should ping @dawehner during the next week.

Just to chek, I've not yet had time to look properly but this jumped to mind as possibly not covered. One of the complexities that was the reason for the relationship (ie explicitly adding the entities to load) was that we don't always want to load entities:

• The item doesn't necessarily have an entity. This is the Nick_vh's external indexed content example. This one might be covered?
• The data required might already be in the search results, so there is no need add an entity load, so it should be explicit when it happens.

Good point, ekes, we definitely need to cover non-entity datasources, too – otherwise Nick is gonna be cross with us.
Same goes for always loading the entity. We'll really have to see whether using the existing Views data tables is compatible with these requirements (at least the second – the first will surely need us to create separate tables), and also with being able to add a second, third, etc. layer of related entities.
It's great if we can completely re-use the table definitions and field handlers, but we shouldn't stick to it dogmatically if it turns out to just lead to too many problems. I also had to do small overrides of the filter plugins for our search filters – most parts could be made made to work without it, but not everything.

I think I will further look into and work on ekes'/Joris' "branch" now and see where that leads. I would love to wait and see whether your idea works, marthinal, but since this is one of the most pressing beta blockers I fear we don't have the time for that and should have a contigency already if it turns out not to be practical after all. So, suboptimal as it is, I think we should just work in parallel and keep the issue here updated with our respective progress.
In any case, most of the work can probably be shared, and I'll first just concentrate on getting non-entity datasources to work, which will probably a) be a lot of work anyways, b) aid me in understanding how this can generally work. and c) be completely independent of your work. Especially Views' existing relationship handler will be interesting to look at in more detail.

Of course, I understand that you need to invest the time correctly and I totally agree with you.

Well, I think that if we have a remote entity defined in our Drupal, then we can instantiate the entity instead of loading it(using #47). In this case if you want to render the complete entity you need all the fields and if you want to render some fields then you only need some fields from the response.

Here I'm hardcoding the type and the title for node entities (SearchApiQuery::addResults()):

      // The entity is used to render fields.
      //$values['_entity'] = $result->getOriginalObject()->getValue();
      $edit_node = array(
        'nid' => NULL,
        'type' => 'article',
        'title' => 'THIS IS A TEST!'
      );

      $values['_entity'] = entity_create('node', $edit_node);

And here is the result:

But I'm not sure if non-entity datasources means a remote entity and if we can format the fields from our Drupal. No idea about this use case to be honest.
Anyway this issue needs a lot of work yes :)

Attached is a general clean-up of the ekes/borisson patch, no (major) functional changes yet.

But I'm not sure if non-entity datasources means a remote entity and if we can format the fields from our Drupal.

No, a non-entity datasource is something else than one for remote entities. In the Search API, the concept of "datasources" is entirely decoupled from Drupal entity types (although, of course, largely based on them). And while we, by default, provide one datasource for every (content) entity type, people are free to create datasources based on whatever data they want.
The question of remote entities is actually interesting in its own right – they can't be loaded normally, but ideally we would create them as entity objects from the field data we receive, which currently isn't really possible. But I guess this is a completely different issue, which can maybe just wait until someone comes with a use case.

In summary, in this issue we need to take the following cases into account:

• The "normal" one: We index entities, get the entity IDs back from the search query and then load the entities to display their fields. (Additional complexity here: also showing fields of related entities, without Views trying to load them via the database query.)
• Indexing entities but getting the fields data back from the server, and not wanting to load entities as long as the field values are there.
• Indexing (or even just searching) non-entity data. Data might still be in the Drupal database, so that items can be loaded, or data is external and we have to solely rely on the field values returned from the search server.

Especially the second points to us not being able to just re-use Views' default field handlers, I'd say? Not completely sure yet, though.
Another problem is click sorting: by default, I think most fields handlers on entities allow click sorting, but we can only allow it for indexed fields. If we re-use the normal Views table data definitions, I don't think there is a way to implement this, we could just throw exceptions whenever someone tries to click sort on a field for which it isn't supported. (Additionally, we can now have multiple Search API fields for the same entity property, and then it also becomes a problem to pick the field we want to use. So maybe we'll even have to provide separate handlers for all indexed fields and only allow click sorting on those.

I am now pretty certain it's not possible to re-use Views' own entity table definitions. They add filters, arguments and sorts the user can't use, options that don't apply, and we need our own system for non-entity datasources anyways (and for entity datasources with no base table, I guess). It's a pity, but I don't see a way around it, and with the use of traits this should at least be somewhat cleaner now than it was in D7. (Also, maybe, with the experience from back then I'll be able to make the system less confusing in general, and make it work more with Views than against it.)

Currently, I'm planning to do the following:

• For each index, provide one base table with filter/argument/sort handlers for all special fields and the handlers for our special fields. To allow click sorting and retrieving the field values directly from the results, also add field handlers for all indexed fields (like in D7).
• For each datasource of each index, add an additional table with field handlers for all its properties and relationship handlers for any entity references. The datasource-specific tables are automatically joined to the base table so that they don't have to be added via relationships manually. (We can easily change that, though, if we find it more confusing than helping.)
• For the relationships of entity references, we also need to add table definitions for each entity type we encounter this way. These tables also have field and relationship handler (possibly requiring additional entity type tables).

I hope the code to provide fitting field handlers won't be too complex, but I'm actually pretty optimistic there. Since ekes already made most of it work without touching any field plugin classes, I guess we should be able to add the missing parts with just a bit of fiddling.

If you disagree with any of this or have other comments, please speak up before I start all the work tomorrow! ;)

Far from done*, but this is a first glimpse into how I envision this could work, as explained above (and, mostly, as seen in D7). At the moment there is only one field handler, without any extra features (except list handling), but that one works quite well, even with nested relationships (e.g., a comment's node's tags' properties). However, linking to nested entities wouldn't be possible right now, since the object loading code always just loads the result object, and can't load the nested ones. I'll have to fix that yet.
Then, I'll move most of the methods from the field handler to a trait, I think, and create field handlers for other types based off of specialized Views field handlers.
Regarding relationships I also have to figure out one problem in particular: I again had to hard-code an additional :entity: separator for their property paths, I'll really have to find a way around that.
Finally, I think there would only be some tests to write. But all in all, this will surely keep me busy for another few days.

Anyone who wants to can take a look, but it's still very much a WIP, so a review would probably be too early. But maybe someone has good pointers?

* Still setting to "Needs review" to see if the test bot is happy – he should actually be, since I haven't yet added any tests and, as far as I know, haven't broken the existing plugins (or, rather, repaired them each time after breaking them).

Posting my current status, still a lot to do. What's working:
- Basic structure with trait is there and seems to be working.
- Loading objects for nested properties works correctly, as does the "Link to its item" option.
- I've added a handler for dates as one example, and it works brilliantly – one extends and one use (plus one implements for multi-value handling), not a single line of code.

The patch also contains a bit of #2646936: Add a helper method for splitting a property path, since I'm using that method already in several places.

Still to do:
- For Field API fields, let users choose between Field API's native widget and our own rendering (to allow entity loading to be skipped when field values are present in the results).
- Make handlers for a few other types (numeric and boolean, at least, and one for properties with options lists (which we should then use for the datasource ID and our language field, at least)).
- Come to think of it: make sure this also works correctly if someone leaves out the implements MultiItemsFieldHandlerInterface part.
- Add access checks back in, both for the immediate result items and any nested entities (probably only the former optional; or all of them, but with a scarier title/description, and defaulting to yes – better safe than sorry (i.e., better a bit of performance loss than data leaks)).
- Expand the Views test to also test field handlers. Don't know how to best do that, either we need two exported views, or we manipulate the view in code, or we even test via the Views UI, making at the same time sure we integrate correctly with that, too.

But that's already it, not that daunting anymore. But still quite a bit of work.

I quickly went trough the patch to see if I can understand it, the answer is currently still no. I did find a couple of smaller issues though.

Regarding .4 and .7, I think you need to rebase your branch.

1. +++ b/search_api.views.inc
   @@ -13,31 +17,57 @@ use Drupal\search_api\Utility;
   + * Also, for each entity type that appears as the target entity type of an
   + * entity reference property in any datasource (or in any entity type thus
   + * reached), a table with field/relationship handlers for all of the entity
   + * type's properties. Those tables will use the key "search_api_entity_ENTITY".
   

   Can we simplify the language of this doc?

2. +++ b/search_api.views.inc
   @@ -321,3 +299,416 @@ function _search_api_views_handler_mapping() {
   +function _search_api_views_get_field_handler_for_property(DataDefinitionInterface $property, $property_path = NULL) {
   

   This method has a bunch of $mapping variables in comment, can those be removed?

3. +++ b/search_api.views.inc
   @@ -321,3 +299,416 @@ function _search_api_views_handler_mapping() {
   +        // Use the "S" modifier for closer analysis of the pattern, since it
   +        // might be executed a lot. (The docs say this won't get us anything in
   +        // our case, but this might change, or be different, e.g., in HHVM.)
   

   This comment is weird. I don't think we need to reference why this might be different. Let's stick to the first part of the comment?

4. +++ b/src/Entity/Index.php
   @@ -1168,10 +1167,6 @@ class Index extends ConfigEntityBase implements IndexInterface {
      protected function reactToServerSwitch(IndexInterface $original) {
   -    // Asserts that the index was enabled before saving and will still be
   -    // enabled afterwards. Otherwise, this method should not be called.
   -    assert('$this->status() && $original->status()', '::reactToServerSwitch should only be called when the index is enabled');
   
   @@ -1198,10 +1193,6 @@ class Index extends ConfigEntityBase implements IndexInterface {
      protected function reactToDatasourceSwitch(IndexInterface $original) {
   ...
   -    assert('$this->status() && $original->status()', '::reactToDatasourceSwitch should only be called when the index is enabled');
   
   @@ -1224,10 +1215,6 @@ class Index extends ConfigEntityBase implements IndexInterface {
      protected function reactToTrackerSwitch(IndexInterface $original) {
   ...
   -    assert('$this->status() && $original->status()', '::reactToTrackerSwitch should only be called when the index is enabled');
   

   These new asserts are removed, I assume this is by accident?

5. +++ b/src/Plugin/views/field/SearchApiFieldTrait.php
   @@ -0,0 +1,530 @@
   +    // @todo Also, this will unnecessarily load items/entities even if all
   +    //   required fields are provided in the results. However, to solve this,
   +    //   expandRequiredProperties() would have to
   

   I think this misses some words.

6. +++ b/src/Plugin/views/field/SearchApiFieldTrait.php
   @@ -0,0 +1,530 @@
   +      // Although it's undocumented, the field handler base class assumes items
   +      // will always be arrays.
   

   Can we file an issue to document this? And link to it, so this documentation doesn't get out of date?

7. +++ b/src/Tests/LanguageIntegrationTest.php
   @@ -18,6 +18,13 @@ use Drupal\search_api\Entity\Index;
      /**
   +   * The ID of the search index used for this test.
   +   *
   +   * @var string
   +   */
   +  protected $indexId;
   +
   
   @@ -39,7 +46,8 @@ class LanguageIntegrationTest extends WebTestBase {
   -    $this->getTestIndex();
   +    $index = $this->getTestIndex();
   +    $this->indexId = $index->id();
   

   This got changed the other way around recently, I'm assuming this was not on purpose?

8. +++ b/src/Tests/ViewsTest.php
   @@ -182,4 +182,19 @@ class ViewsTest extends WebTestBase {
   +    $this->drupalGet('admin/structure/views/view/search_api_test_views_fulltext');
   

   We should probably add a $this->assertResponse(200); . This is the very least we can do to ensure that we're not introducing major breaks before we add a specific test for this.

You're right, forgot to rebase. Sorry!

I quickly went trough the patch to see if I can understand it, the answer is currently still no.

Anything I can do to help? Or should we wait until the patch is finished? (You can also ping me in IRC about this, perhaps that would be easier.)

Can we simplify the language of this doc?

I don't know, is this better?

Also, for each entity type encountered in any table, a table with field/relationship handlers for all of that entity type's properties is created. Those tables will use the key "search_api_entity_ENTITY".

Otherwise, what's your suggestion?

This method has a bunch of $mapping variables in comment, can those be removed?

They certainly won't be there in this form in the final patch – but since we probably want to really add handlers for each of them, I kept those comments there for the moment, so that I just need to remove the comment marks later. (Since I already did the work of looking up the different field types.)

To Camelize or not, that is the question.

Do you mean anything by that, or is it just a joke?

If I can test it out, let me know - happy to do so.

Well, most of what's there is already working. But I guess it would still make sense to wait until I'm finished with the patch so far, so that you don't waste time documenting missing functionality that's already planned.

I'll clearly say it once its time for testing and final reviews.

In the meantime, the attached patch is properly rebased and contains fixes for Joris' comments in #66.

It should also fix the (existing) tests, by re-introducing the entity_access query setting to the schema, for the moment.
However, since we will probably have to check access in the field handlers anyways, maybe we should move the option there?

My current idea: Have checkboxes for "Skip access checks" on both the query and the fields. The query only includes those result items that pass the access checks in the results array (unless checks are skipped).
Then, based on those results, the field handlers will check each nested entity they're accessing for retrieving the property (if any). The option could be hidden if there is no colon (:) in the property path (i.e., it's a direct property of the result item), in which case no access checks would be performed by the field handler anyways.
Does that make sense, in your opinion?

I quickly went trough the patch to see if I can understand it, the answer is currently still no.

Anything I can do to help? Or should we wait until the patch is finished? (You can also ping me in IRC about this, perhaps that would be easier.)

Let's wait until the patch is in a better state.

Can we simplify the language of this doc?

I don't know, is this better?

Yes, looks great.

This method has a bunch of $mapping variables in comment, can those be removed?

They certainly won't be there in this form in the final patch – but since we probably want to really add handlers for each of them, I kept those comments there for the moment, so that I just need to remove the comment marks later. (Since I already did the work of looking up the different field types.)

Ok, good.

My current idea: Have checkboxes for "Skip access checks" on both the query and the fields. The query only includes those result items that pass the access checks in the results array (unless checks are skipped).
Then, based on those results, the field handlers will check each nested entity they're accessing for retrieving the property (if any). The option could be hidden if there is no colon (:) in the property path (i.e., it's a direct property of the result item), in which case no access checks would be performed by the field handler anyways.
Does that make sense, in your opinion?

That makes sense, yeah.

I ran into some problems implementing "Skip access checks" for field handlers, so the access checks are now non-optional, at least at that level. I added #2650364: Add a "skip access checks" option to the Views relationship plugin to change that, and also #2650270: Add access checks to datasources to be able to do result-level access checks even on non-entities.

Apart from that, though, I think this patch is ready to be reviewed. As laid out above, I had to use a lot of workarounds and ugly compromises, both in the UI and in the code, but all the functionality should be there, and I'm available for discussing any potential improvements.

If someone wants a detailed explanation of what goes on internally, please say so (if possible specifying which part you don't understand). Otherwise, please just give it a spin and see whether everything works as expected, or whether you can break anything. I think I tested it rather thoroughly, but I'm sure there are still things not working as they should.

In any case, the tests are still missing, I'm now gonna work on those so that we can even be more confident that this works as it should.

I looked at the interdiff, will take a look at the complete patch next week. I found a couple of small coding standards / comments remarks.

1. +++ b/search_api.views.inc
   @@ -593,6 +593,50 @@ function _search_api_views_add_handlers_for_properties(array $properties, array
   +  // If there is a definition and the property represents a Field API field, add
   +  // the "field_name" key.
   

   This is a really good comment

2. +++ b/search_api.views.inc
   @@ -637,16 +676,34 @@ function _search_api_views_get_field_handler_for_property(DataDefinitionInterfac
   +    // Field items have a special handler, but need a fallback handler set to be
   +    // able to optionally circumvent entity field rendering. That's why we just
   +    // set the "field_item:…" types to their fallback handlers above, along with
   +    // non-field item types, and here post-process them to correct that.
   

   I don't understand what "post-process" means in this context.

3. +++ b/src/Plugin/views/field/SearchApiBoolean.php
   @@ -0,0 +1,24 @@
   +class SearchApiBoolean extends Boolean implements MultiItemsFieldHandlerInterface{
   

   Coding standards: needs a space before {

4. +++ b/src/Plugin/views/field/SearchApiEntity.php
   @@ -0,0 +1,276 @@
   +      if ($this->options['link_to_item']) {
   +        $item['make_link'] = TRUE;
   +        $item['url'] = $entity->toUrl('canonical');
   +      }
   

   instead of doing this, can we return an instance of \Drupal\Core\Link as $item['value'] here? That also renders into a render array (see Link::toRenderable). The method docblock should be updated as well I guess, but this makes the "possible also additional alter options" unneeded.

5. +++ b/src/Plugin/views/field/SearchApiEntityField.php
   @@ -0,0 +1,291 @@
   +    $property_path = $parent_path ? "$parent_path:_object" : '_object';
   

   Can we make the string concatenation here more explicit by changing it to $parent_path . ':_object'. I know that the coding standards allow this, so it's just taste.

6. +++ b/src/Plugin/views/field/SearchApiEntityField.php
   @@ -0,0 +1,291 @@
   +    // For some strange reason, the boolean formatter hard-codes the field name
   +    // to "field_boolean", breaking our parent class's rewriteStatesSelector()
   +    // call for fixing "#states". Therefore, we just apply that here again, with
   +    // the hard-coded field name.
   +    if (!empty($form['settings'])) {
   +      FormHelper::rewriteStatesSelector($form['settings'], "fields[field_boolean][settings_edit_form]", 'options');
   +    }
   

   I think this comment can be rewritten to be a little better; I'm sure someone has thought about the core implementation a lot and there'll be a reason for that.

   The core boolean formatter hard-codes the field name to "field_boolean". This breaks our own implementation of rewriteStatesSelector() to fix "#states". We apply that behavior again here.

7. +++ b/src/Plugin/views/field/SearchApiEntityField.php
   @@ -0,0 +1,291 @@
   +
   +
   +
   

   This can be reduced to 1 newline.

8. +++ b/src/Plugin/views/field/SearchApiEntityField.php
   @@ -0,0 +1,291 @@
   +    // Same for the options form from the fallback handler, but here we only
   +    // add the options not already present, and put the reverse "#states"
   +    // directive on them.
   

   I think we can rewrite this comment as well so it doesn't start with "Same".

9. +++ b/src/Plugin/views/field/SearchApiFieldTrait.php
   @@ -193,6 +224,45 @@ trait SearchApiFieldTrait {
   +    // @todo This will work in most cases, but might fail for multi-valued
   +    //   fields.
   

   I can see that this is a todo, but let's remember to write a testcase for multi-valued fields.

10. +++ b/src/Plugin/views/field/SearchApiStandard.php
    @@ -16,7 +17,7 @@ use Drupal\views\Plugin\views\field\PrerenderList;
    +class SearchApiStandard extends FieldPluginBase implements MultiItemsFieldHandlerInterface{
    

    Coding standards; space before {

instead of doing this, can we return an instance of \Drupal\Core\Link as $item['value'] here? That also renders into a render array (see Link::toRenderable). The method docblock should be updated as well I guess, but this makes the "possible also additional alter options" unneeded.

I guess we could, but that's not really how Views works, and it might lead to any number of unintended consquences. I'd rather stay in Views' pattern, that way things tend to Just Work™ – even if you usually don't understand why.

Can we make the string concatenation here more explicit by changing it to $parent_path . ':_object'. I know that the coding standards allow this, so it's just taste.

Normally, I'd agree, but in this case I think it makes the ternary operator more confusing. I think with syntax highlighting it's clear enough this way.

I think this comment can be rewritten to be a little better; I'm sure someone has thought about the core implementation a lot and there'll be a reason for that.

You obviously place a great deal more trust into Core development than I do.
But sure, no need to include such rants in code comments.

The core boolean formatter hard-codes the field name to "field_boolean". This breaks our own implementation of rewriteStatesSelector() to fix "#states". We apply that behavior again here.

It's not "our" implementation, it's the call of the (Core Views) parent class.
I quickly took a look, and it really does seem to be a Core bug. And since I knew what your next question would be – here it is: #2650994: Javascript states not working for boolean fields.

Coding standards; space before {

Oh, twice?!? I don't know why PhpStorm keeps doing that. If I tell it to format, it correctly adds a space there, but it deletes it when I'm manually adding an implements clause there.
Anyways, thanks for catching both of those!

See the attached patch for the corrections. The tests will probably only come after my vacation.

Awesome, thanks for the response, I agree with all your remarks.

Getting an error in line 415 of patch in #79.

It should probably be $row->{$combined_property_path}[] instead of $row->$combined_property_path[]

Getting an error in line 415 of patch in #79.

It should probably be $row->{$combined_property_path}[] instead of $row->$combined_property_path[]

Thanks for noting that, makes sense. I think handling might differ there between PHP versions, so maybe that's why it worked fine for me. However, being explicit can't do any harm in any case.

Anyways, rejoice, for I think this patch is RTBC now! It adds proper tests for most of the field handlers and they pass fine for me locally.

1. +++ b/src/Tests/ViewsTest.php
   @@ -50,7 +51,7 @@ class ViewsTest extends WebTestBase {
   -  public function testView() {
   +  public function dpmtestView() {
   

   I think you added this during dev and forgot to remove the "dpm"?

Otherwise the changes look great.

Oops, completely correct of course. Thanks for spotting that!
Also, the commented-out test now failed, so fixing that, too, along with the strict warnings in PHP 5.

One more strict warning to go before this is ready!

I'm happy with the latest changes. I'd say this is RTBC.

Oh. Thanks a lot for spotting that! Seems I introduced this in one of my last quick fixes (for PHP 5 strict warning), and we didn't yet have any entity reference fields for testing. I added a test for that, even though not totally complete, and also fixed the problem.

Two more follow-ups:

• #2657406: Add tests for Views fields from relationship
• #2657408: Avoid unnecessary entity loads for entity reference Views fields

I also changed the help for properties with missing descriptions and added a "Raw ID" option for entity reference field handlers.

And with that, I hope this is finally RTBC?

There's a testmethod that's still has the dpm prefix, can we run tests again without that?

Ah, sorry, that was just in the interdiff. The other two don't have it, I noticed in when creating the tests-only patch.
So, RTBC?

Got the OK from both Nick and Joris via IRC, so: committed.
Thanks, everyone, for the great work in this issue!