Comments

muschpusch’s picture

drunken monkey’s picture

Status: Active » Postponed (maintainer needs more info)

While I know of the existence and rough purpose of VBO, I know little more. Therefore, I don't really know what you want from me.
What would I have to do to integrate with it? What would be the effect of that? A bit more explanation, please.

Also, if this isn't trivial, I doubt that I'll find the time for this any time soon. You could provide a patch, though – if it isn't too big a deal, I'd probably add it to the module.
On the other hand, if the basic VBO module doesn't work at the moment, now maybe isn't a good time to try to integrate with it, is it?

muschpusch’s picture

Hey, hey! I'm not really good at views development but with a little help i would love to try it. To start with think a combination of search_api and VBO would be great because you could easily search & annotate content. That's not that interesting for websites but really nice for all kind of applications with a lot of data.

VBO has a hook to register new objects: hook_views_bulk_operations_object_info()

// this an example for integration with the data module.
$object_info = array(
    'example_data_table' => array(
      'type' => 'example',
      'base_table' => 'example_data_table', //Must be Data table name.
      'load' => 'example_load_row', //Function which returns a single row
      'oid' => 'id', //Specific column used as your ID.
      'title' => 'name',
    ),
  );

What would be the functions for getting the right table name oid etc? Would this actually work for search_api handling different external datasources??? There is a patch of damien #1123364: Implement hook_views_bulk_operations_object_info() for entities which automatically loads all entities to VBO but search_api data sources aren't entities. Are they????

drunken monkey’s picture

but search_api data sources aren't entities. Are they????

Yes, they are, for now.

Table names are 'search_api_index_' . $index->machine_name, the corresponding entity type is $index->entity_type. Use $indexes = search_api_index_load_multiple(FALSE); to get all indexes. In Views results, $row['entity'] always contains the complete loaded entity.
I don't know what else you'd need, but ask away!

drunken monkey’s picture

Status: Postponed (maintainer needs more info) » Active
muschpusch’s picture

Status: Active » Closed (duplicate)
fangel’s picture

With the latest -alpha3 of Views Bulk Operations, I'm still not able to add VBO fields to Search API Based views. So I'm not sure the efforts in the VBO issue paid out. Can someone confirm if they can get it to work?

fangel’s picture

Status: Closed (duplicate) » Active

Ups, forgot to mark as active again.

timebinder’s picture

Can someone tell me what are all the arguments/configuration and valid values in the
hook_views_bulk_operations_object_info() object definition shown above (in #3)?

I've been trying to find this out to no avail

bojanz’s picture

Status: Active » Closed (won't fix)

I was curious about Search API and VBO together, so I peeked at the code.
So, VBO no longer has hook_views_bulk_operations_object_info(), it supports entities directly.
The VBO field is not visible because it looks for the entity base table in the views data.
Search API defines the base table for the index as "search_api_index", but adds the views data to 'search_api_index_' . $index->machine_name;

I'm guessing there's a good reason for that, so you could just patch views_bulk_operations.views.inc and views_bulk_operations_handler_field_operations::get_entity_type() to look for the "entity type" key as well, instead of just guessing by comparing base tables. It's an emerging Entity API convention, so I'm fine with having it in VBO.
I don't use Search API so I don't know if that would be enough for VBO to work with it, but all it takes is for someone to try.
See you in the VBO queue.

Closing this issue since I don't see anything that should be done on the Search API side (feel free to correct me)

drunken monkey’s picture

I'm guessing there's a good reason for that, so you could just patch views_bulk_operations.views.inc and views_bulk_operations_handler_field_operations::get_entity_type() to look for the "entity type" key as well, instead of just guessing by comparing base tables. It's an emerging Entity API convention, so I'm fine with having it in VBO.
I don't use Search API so I don't know if that would be enough for VBO to work with it, but all it takes is for someone to try.

See #1172970: provide a unified way to retrieve result entities – this convention is even going into Views itself now, along with a unified way to retrieve the entities belonging to a result set. So if it doesn't work with Search API now, it will surely after that issue gets committed (and the Search API is adapted accordingly, but I plan to do that right away, then).

bojanz’s picture

Great, looking forward to that.

sonar_un’s picture

Subscribing, I think this is very important. Thanks DM!

bitsgrecco’s picture

Great feature. Subscribing

bitsgrecco’s picture

Hi all,
I got VBO working with SearchAPI with a couple of workarounds.
I guess it is not the proper way to make them work, but until they are well integrated it seems to work.

Using modules:

  • views 7.x-3.0-rc1
  • views_bulk_operations 7.x-3.0-beta2
  • search_api 7.x-1.0-beta10

Applied changes:
* On file views_bulk_operations/views/views_bulk_operations.views.inc
I added this custom code to the end of function views_bulk_operations_views_data_alter

      $data['search_api_index_<HERE_YOUR_INDEX_NAME>']['views_bulk_operations'] = array(
        'title' => t('Bulk operations'),
        'help' => t('Provide a checkbox to select the row for bulk operations.'),
        'real field' => 'id',
        'field' => array(
          'handler' => 'views_bulk_operations_handler_field_operations',
          'click sortable' => FALSE,
        ),
      );

* On file search_api/contrib/search_api_views/includes/query.inc
I changed
class SearchApiViewsQuery extends views_plugin_query {
to
class SearchApiViewsQuery extends views_plugin_query_default {

This is quite a custom scenario, with an index of nodes.
Looking forward for fully integration between VBO and SearchAPI

bitsgrecco’s picture

FileSize
3.49 KB

Until this integration is implemented into VBO and/or search_api modules,
I wrote this little module to let them work.

I am using following modules:

  • views 7.x-3.0-rc1
  • views_bulk_operations 7.x-3.0-beta2
  • search_api 7.x-1.0-beta10

In order to make it work, there is a little little patch to be done on search_api_views:
On file search_api/contrib/search_api_views/includes/query.inc
I changed
class SearchApiViewsQuery extends views_plugin_query {
to
class SearchApiViewsQuery extends views_plugin_query_default {

bojanz’s picture

@bitsdan
If you add return "searchapi_index_whatever" (or whatever the exact entity type is) at the beginning of views_bulk_operations_field_operations::get_entity_type(), does it still require changing the SearchApiViewsQuery line in SearchAPI?

drunken monkey’s picture

Title: integration with views bulk operations (vbo) » Integration with views bulk operations (vbo)
Status: Closed (won't fix) » Active

If you want to discuss this here (which might make sense when changes to Search API are required after all), please re-open it.

And I think the changed superclass is to avoid the fatal error that the missing add_field() method causes, right?
If that is the case, just adding this method would be a better solution, though. Ultimately it should probably be fixed in VBO, though, as it shouldn't assume the presence of that method—unless it only aims to work with the default Views query plugin, of course, which might be the case. I don't know enough about VBO to judge that.

bitsgrecco’s picture

@bojanz
In order to make the VBO actions work loading the entities I also needed to overwrite method
views_bulk_operations_field_operations::get_entity_type() to return the real entity type the index was based on. But still I require to change the base class of SearchApiViewsQuery to make things work.

@drunken monkey
Yes, changing the superclass is to avoid the fatal error that the missing add_field() method causes. And this method is still invoked even overwritting views_bulk_operations_field_operations::get_entity_type() as mentioned above. I dont know in detail neither VBO nor SearchAPI, just found these a temporary solution to make them work together.

bitsgrecco’s picture

FileSize
3.67 KB

I found that when submitting any VBO action, nothing was happening because entities refered by selected rows couldn't be loaded. This was due to given entity_type on handler search_api_vbo_handler_field_search_api_vbo.

In order to meke them work, I had to overwrite method search_api_vbo_handler_field_search_api_vbo::get_entity_type() returning the right entity type the SearchAPI index was based on.
This required to extend the view's handler definition on method search_api_vbo_views_data_alter(&$data) at file search_api_vbo/views/search_api_vbo.views.inc, taking the entity type from the view's definition of this SearchAPI index.

  $data[$index_views_key]['views_bulk_operations'] = array(
    'title' => t('Bulk operations'),
    'help' => t('Provide a checkbox to select the row for bulk operations.'),
    'real field' => $search_api_index_entity_info['entity keys']['id'],
    'field' => array(
      'handler' => 'search_api_vbo_handler_field_search_api_vbo',
      'click sortable' => FALSE,
      //##############
      'search_api_item_id_field' => 'search_api_item_id',
      'search_api_item_entity_type' => (isset($data[$index_views_key]['table']['base']['entity type']) ? $data[$index_views_key]['table']['base']['entity type'] : NULL),
      //##############
    ),
  );

Although this can work, I dont know enough VBO and SearchAPI to find the right way to integrate them in a well designed solution.
I keep looking forward for a VBO and SearchAPI release integrating together in a normal way.

rbruhn’s picture

Subscribing
@drunken monkey: Now that #1172970: provide a unified way to retrieve result entities is committed to dev, any plan to adapt Search API soon as mentioned in your comment above?
I'm interested in using VBO on indexes as well. Played around with the code yesterday, and can get the VBO field to show on indexes by using hook_view_data_alter() in my module, but get the warning you spoke about above concerning add_field():

call to undefined method SearchApiViewsQuery::add_field()
views/handlers/views_handler_field.inc line 65

I don't understand your response above about adding the method being a better solution. Unfortunately, not all that versed on all the views code either.

bojanz’s picture

Ultimately it should probably be fixed in VBO, though, as it shouldn't assume the presence of that method—unless it only aims to work with the default Views query plugin, of course, which might be the case. I don't know enough about VBO to judge that.

Yes, VBO assumes the default Views query plugin. Not really sure if we can support others with the same code, so maybe we need another class? And abstracting out the part that assumes the default query plugin so that it can be easily overridden. Haven't looked into Search API yet (that will have to wait after VBO 7.x-3.0-RC1 and there's still a lot of work to be done in that direction).

drunken monkey’s picture

@drunken monkey: Now that #1172970: provide a unified way to retrieve result entities is committed to dev, any plan to adapt Search API soon as mentioned in your comment above?

Yes – see #1231512: Use real Relationships instead of level magic in Views integration and #1266036: Add generic Views entity tables with fields and relationships.

Yes, VBO assumes the default Views query plugin. Not really sure if we can support others with the same code, so maybe we need another class? And abstracting out the part that assumes the default query plugin so that it can be easily overridden. Haven't looked into Search API yet (that will have to wait after VBO 7.x-3.0-RC1 and there's still a lot of work to be done in that direction).

As I haven't looked into VBO, I can't say anything really, either. However, when people say that things work with just that method added, maybe calling this method conditionally (on method_exists()) would suffice?

bojanz’s picture

Well, the error actually comes from views_handler_field::query(), so if you can override that method in the VBO field handler, and still have it work (views_form() gets entity ids from $this->view->result), then it might be less work than anticipated (there's still the question of doing it cleanly, but oh well...)

bojanz’s picture

Well, the error actually comes from views_handler_field::query(), so if you can override that method in the VBO field handler, and still have it work (views_form() gets entity ids from $this->view->result), then it might be less work than anticipated (there's still the question of doing it cleanly, but oh well...)

drunken monkey’s picture

FileSize
1.48 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1123454--add_field.patch. Unable to apply patch. See the log in the details link for more information. View

Let's just try it with the attached patch, which renames Search API's method. Please test whether this already resolves the issue!

drunken monkey’s picture

Status: Active » Needs review
bojanz’s picture

function add_field($table, $field, $alias = '', $params = array()) {

The signature is not even remotely the same...

drunken monkey’s picture

FileSize
516 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1123454--add_field-29.patch. Unable to apply patch. See the log in the details link for more information. View

Of course, sorry for that.

rbruhn’s picture

I tried out the last patch. Simply adding the add_field() function, right?
In doing what I did before, I added this to my module:

function bir_views_data_alter(&$data) {
  if (isset($data['search_api_index'])) {
    foreach (search_api_index_load_multiple(FALSE) as $index) {
      // Base data
      $key = 'search_api_index_' . $index->machine_name;
      $data[$key]['views_bulk_operations'] = array(
        'title' => t('Bulk operations'),
        'help' => t('Provide a checkbox to select the row for bulk operations.'),
        'real field' => 'id',
        'field' => array(
          'handler' => 'views_bulk_operations_handler_field_operations',
          'click sortable' => FALSE,
        ),
      );
    }
  }
}

Still the only way to get bulk operations field to show.
I'm able to add the field now without the add_field() error. Specific to my view, I'm using an index where the only available bulk action is:

Node: Bulk operations
Provide a checkbox to select the row for bulk operations

The particular index I'm using has node as the entity_type.
When selecting a check box and executing the bulk operation, I receive an error stating I need to select something. In looking at the html, it is probably due to the value of the checkboxes being empty:

<input class="vbo-select form-checkbox viewsImplicitFormSubmission-processed" type="checkbox" id="edit-views-bulk-operations-528" name="views_bulk_operations[528]" value="">

I hope this helps mean something to someone :-)

bojanz’s picture

That means that #29 is not enough. Still, it's definitely a step in the right direction.

I guess that the code in views_bulk_operations_handler_field_operations::views_form:

foreach ($this->view->result as $row_index => $row) {
      $entity_id = $this->get_value($row);

fails.

rbruhn’s picture

FileSize
194.72 KB
21.88 KB

@bojanz
Finally had some time to take a look at this again.
I wanted to understand what was happening in regards to post #31 and the views_bulk_operations_handler_field_operations::views_form method.

I dumped out the results of $row and attached images to this comment so you might see the differences. Perhaps it might give you an idea where to point me to next. It's obvious the results are different.

In looking at the $entity_id = $this->get_value($row); within the views_form method, it appears $entity_id is not being populated. I'm really not sure why. Perhaps you can give me a clue.

The image labeled normal is using a view selecting Content: Bulk operations in the field list.
The image search_index is used using a view based on the solr index and selecting Node - Bulk Operations. That is the only bulk operation field that shows up in the field list (obviously because I'm adding it in my views_data_alter).

I hope this helps. Would appreciate any more pointers to get this working.

rbruhn’s picture

A follow up on the above.
In my post #30 above, I used 'real field' => 'id',
If I use 'real field' => 'nid' then the checbox input has the correct value (the node ids). However, when submitting the form for executing arbitrary php code, it throws an error due to the entity_type being blank.

_views_bulk_operations_entity_load($entity_type, $ids, $revision = FALSE)

This appears to be a problem associated with views_bulk_operations_form_submit() and what values are being passed.
If it was possible to tell the form what entity_type is being used in the solr index table (node in my case), it might solve the problem. I'm still trying to figure out exactly what form value is needed ... the form['values']['operation'] ?

mh86’s picture

added a new issue to the vbo queue (#1334374: Re-use generic entity views table), as I think it should be generically implemented there (which is possible with the latest Views updates)

drewish’s picture

So in addition to the patch on #29 I took what rbruhn had been working on and then started trying to hack around the errors. The easiest way I could think of to get the entity type in was extend their field handler and just pass the type along in the field definition. That got me to the part where it started complaining about the entity id field not existing so I just did some more hacking to override the get_value() method and pulled my best guess value out:

function hacky_search_views_data_alter(&$data) {
  if (isset($data['search_api_index'])) {
    foreach (search_api_index_load_multiple(FALSE) as $index) {
      $data['search_api_index_' . $index->machine_name]['views_bulk_operations'] = array(
        'title' => t('Bulk operations'),
        'help' => t('Provide a checkbox to select the row for bulk operations.'),
        'real field' => 'id',
        'field' => array(
          'handler' => 'hacky_search_api_vbo_handler_field_operations',
          'item_type' => $index->item_type,
          'click sortable' => FALSE,
        ),
      );
    }
  }
}

class hacky_search_api_vbo_handler_field_operations extends views_bulk_operations_handler_field_operations {
  /**
   * Override their get entity type since the base table name won't match at all.
   */
  function get_entity_type() {
    return $this->definition['item_type'];
  }

  /**
   * Overridden to try to fish out the id.
   */
  public function get_value($values, $field = NULL) {
    // I'm not sure this is the best source for this but the name seemed consistent.
    return $values->_entity_properties['search_api_item_id'];
  }
}

I feel like there's got to be a cleaner way to do this but this lets me close my ticket.

rooby’s picture

FileSize
1.42 KB
PASSED: [[SimpleTest]]: [MySQL] 298 pass(es). View

Here is a redo of the patch in #29 with commenting.

I am now able to use search API & VBO using:
* Latest dev version of views (needs recently committed views patches)
* Latest dev of VBO with the patch at #1334374-9: Re-use generic entity views table
* Search API with this patch.
* No additional custom code or hacks :)

rooby’s picture

Note, I have only tested a node index so far.

bojanz’s picture

I've created a VBO documentation page that holds the code provided by drewish: http://drupal.org/node/1591360.
Would be great if someone could create a sandbox with the full module, for people to use until this gets addressed properly.

jlyon’s picture

I get a WSOD when I try to run #36 with users.

rooby’s picture

I get a WSOD when I try to run #36 with users.

Any chance you can get the error message from your server log?

Do you also have the other patch mentioned in #36? And have cleared your drupal caches?

yanniboi’s picture

Hey everyone,

The above is hugely helpful. Thanks to everyone who has been working on it :)

@drunkenmonkey - Quick question. Is the add_field() patch ever going to get committed? (I think #36 is outdated by now, but something of the sort). We are using this on a production site and as everything else can be done quite tidily we would rather not have hacked modules on it.

Ta!

yanniboi’s picture

Status: Needs review » Needs work
yanniboi’s picture

Status: Needs work » Reviewed & tested by the community

Sorry I meant to set it to reviewed and tested, not needs work...

bojanz’s picture

Status: Reviewed & tested by the community » Needs review

#36 should not be necessary with the right VBO patch.

yanniboi’s picture

What do you mean bojanz? What is the 'right VBO patch'.

rooby’s picture

There isn't one yet as far as I know.

Although this probably should be won't fix if it isn't necessary.

mh86’s picture

IMO right patch: the generic entity views handler needs to support revisions (see #1334374: Re-use generic entity views table for more info)

dmatamales’s picture

So what should we do for right now?

I can't get it to work without #36 above. With #36 and #1334374-11 it seems to be working great, but there are lots of notices and warnings coming up for the fuzzysearch module (which uses Search API). But with error message display turned off, everything goes smoothly, so I'm fine with using it for now.

Is there a better solution for right now than those two patches to get VBO working with Search API?

rlmumford’s picture

Status: Needs review » Needs work

Trying to make this work with large data sets and encountering alot of problems. At the moment this seems incapable of doing operations on > 5000 items. We need to do operations on > 100000 items. I imagine this is because the SOLR Query is returning loads of fields that aren;t needed by VBO when preparing the queue. Can we think of a way of getting SOLR to only give the ids.

TravisCarden’s picture

It sounds like some people have this working at least provisionally, but I haven't been able to get it so for myself. Could anyone indicate what combination of module versions and patches to use?

drunken monkey’s picture

Trying to make this work with large data sets and encountering alot of problems. At the moment this seems incapable of doing operations on > 5000 items. We need to do operations on > 100000 items. I imagine this is because the SOLR Query is returning loads of fields that aren;t needed by VBO when preparing the queue. Can we think of a way of getting SOLR to only give the ids.

Uncheck "Retrieve result data from Solr" in the server settings.

andrewbelcher’s picture

FileSize
37.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1123454-search_api-vbo-integration.patch. Unable to apply patch. See the log in the details link for more information. View

Uncheck "Retrieve result data from Solr" in the server settings.

With that, is there anything that still needs work on it?

It might be helpful to make it so that you can turn off result retrieval for specific queries (in fact that could already be done using hook_search_api_solr_query_alter()... Should that be included in this patch? As we don't even need the score for VBO, just the item_id.

I've attached the patch re-rolled against head... Would be great to get this one in!

andrewbelcher’s picture

Ok, looks like the patch I was using also includes a handler for VBO that the other patches don't... Where has everybody else got their VBO handler from? Does VBO add a handler for Search API indexes?

drunken monkey’s picture

Your patch is broken, you seem to have diffed the current dev version against 1.7 to create it. Please create a clean patch, verify that it applies cleanly to the latest dev version and that with just applying it to the dev version VBO with Search API works. (Or mention here what else is needed.)

frankcarey’s picture

Status: Needs work » Needs review

#36: search_api-vbo-1123454-36.patch queued for re-testing.

frankcarey’s picture

comment #36 is working for me:

Reposting the updated details here, but the patch didn't need to be rerolled as far as I can tell (this is a simple patch). I requested a retest.

I am now able to use search API & VBO using:
* Views 7.x-3.7 (includes the necessary patches)
* Latest dev of VBO with the patch at #1334374-9: Re-use generic entity views table
* Search API with patch from #36. (I used the latest dev)
* No additional custom code or hacks :)

Status: Needs review » Needs work

The last submitted patch, 1123454-search_api-vbo-integration.patch, failed testing.

drunken monkey’s picture

Well, in any case, the method documentation for add_field() should be properly adapted.

However, I'm still more of bonjanz's opinion: adding a (more or less) dummy method shouldn't be necessary, and while it would maybe result in a quicker fix for this problem now, it would hide bugs in other modules (which assume all views use the database) in the future.
For the moment, let's see what version of #1334374: Re-use generic entity views table will get committed, and then base our decision on that.

Iztok’s picture

I can confirm that #56 works on the latest stable releases.

almc’s picture

Unfortunately, it doesn't work well, at least how I see it for a custom entity. I see checkboxes appeared but unable to configure a meaningful action on them, and what I need is just to collect ids from the checked lines and execute a custom PHP script/function for those ids in the background.

andrewbelcher’s picture

Issue summary: View changes

I've been looking at this a bit last night/this morning. With the latest patch from #1334374: Re-use generic entity views table, it is working without needing to patch Search API as we remove the dependency on $field_alias which was what was causing the problem.

If that patch get's committed then I think this can be closed with no action!

drunken monkey’s picture

Excellent, that's great news! Thanks for posting this here, I hope this will be fixed soon.

benjarlett’s picture

has this been committed? or do I need to patch it? I need to bulk delete a bunch of submissions with vbo.

stickywes’s picture

I think we're still waiting to see what happens with #1334374: Re-use generic entity views table as far as any committing goes. You'll have to see if the patch helps you for now.

drunken monkey’s picture

damien_vancouver’s picture

I've successfully got VBO + search_api working.

I had to first patch VBO with patch #25 from #1334374: Re-use generic entity views table against VBO 3.x-7.2 release.

Then I added Drewish's code from #35 on this thread (also found at doc page https://www.drupal.org/node/1591360) to a custom module and finally cache clears of everything and I was able to add VBO field handlers to my view.

There are a couple other reports of success on that thread as well. Perhaps if we can get that patch committed, then look at getting the code from #35 either into a patch for search_api itself, or get that sandbox helper module promoted to real module status so it's something people can just install to add support.

mrconnerton’s picture

I did the same as damien_vancouver in #66 and for the most part it is working. I think I'm seeing some inconsistencies with the "Select all rows in the view". I'm using modify entity values operations. If I have view with 32 rows, 25 per page, after I select all and run the operation the last 7 are not being updated

dready2011’s picture

I have the same situation as mrconnerton: it works almost perfectly, but the "Select all rows in the view" funtionality is not giving the correct results. In my case, too many records are being handled. Let's say I have a pager with 25 results per page, and the view returns 32 results. After selecting "Select all rows in the view" more then 32 rows are updated through my vbo action.
I must add, that I use a combination of exposed filters and facets.
Any idea where to start looking for a solution to this problem?

andrewbelcher’s picture

Status: Needs work » Needs review
FileSize
1.17 KB

I'm pretty sure with the patch from #1334374-25: Re-use generic entity views table, the attached patch is all that is needed, not all the additional code that is done in #35. I've tested it and it looks to work as expected for me.

@dready2011 - there is no current support for FacetAPI as they don't get applied through views and so work completely separately. I would suggest that could be dealt with in a follow up..?

drunken monkey’s picture

Thanks a lot, andrewbelcher! If that's indeed all that's needed, that would of course be great. If someone could verify this works for them, I'll gladly commit it. (Seems the VBO issue is already quite stable, so hopefully it will be compatible with what gets committed.)
Or should we maybe include a bit of safe-guarding code to prevent problems when people are using this together with older VBO versions?

andrewbelcher’s picture

I think some kind of safeguarding would be wise, otherwise we'd probably have to wait for the next tagged release after it's committed. We could do something along the lines of adding a hook_views_data_alter() (and hook_module_implements_alter() to ensure it runs after views_bulk_operations_views_data_alter()) and remove our VBO handler if the VBO handler is on the base table rather than the views_entity_ENTITY_TYPE table. We could even move the code that adds the handler in the first place to there.

This has the added benefit of it would "just work" with a patched version of VBO as well... We can document to remove it when we VBO tags a release with the patch included.

Let me know if you like that idea and I can update the patch, or feel free to suggest an alternative!

drunken monkey’s picture

Yes, that does sound like a pretty good way to fix this, thanks!
I'd just move the whole code you're adding to the alter hook, then – no sense in first adding and then removing it.
(One idea I had when I skimmed the VBO patch is checking for the parent class of views_bulk_operations_handler_field_operations, but your solution seems much more to the point.)

drunken monkey’s picture

Status: Needs review » Needs work
andrewbelcher’s picture

Status: Needs work » Needs review
FileSize
2.58 KB

I've not had a chance to test this yet - hope to do tomorrow, but here it is for you to check the approach/logic.

andrewbelcher’s picture

FileSize
4.03 KB

Good job I tested, I missed one thing - VBO (and also incidentally Entity Reference) calls $view->query->add_field(). This is not part of the interface, but both assume that it exists (or assume that they are working with the default query engine)...

The simple workaround is to add it as a wrapper around addField()! I've updated the patch to include this.

drunken monkey’s picture

Good job I tested, I missed one thing - VBO (and also incidentally Entity Reference) calls $view-&gt;query-&gt;add_field(). This is not part of the interface, but both assume that it exists (or assume that they are working with the default query engine)...

The simple workaround is to add it as a wrapper around addField()! I've updated the patch to include this.

What happens when you instead skip that method call if the method isn't present on the query plugin? Does it still work, or is the call required for Search API views, too?
In general, I've avoided adding dummy/proxy methods to the query plugin so far (at least in D7 – in D8 I had to give in, since they kept the same broken code for another major release cycle) and would love it if the solution for this could go into VBO along with #1334374: Re-use generic entity views table instead.
If the call is required, though, I guess we have little choice. (Adding a single method_exists() to VBO seems clean enough, but having them test through various variations of the method doesn't.)

andrewbelcher’s picture

Vbo would have to be aware of the different variations that exist to support different things, as would entity reference which does the same thing...

To me this looks like either views isn't interfacing a method it should (the alternative bein other ecosystem modules assume a method is interfaced when it's isn't and shouldn't be). This is already an issue with 2 other modules, so my inclination is to put the proxy in place for modules that assume it's there...

But I understand the hesitation! It's really a views issue, but views interfacing it could cause fatal errors in other modules that are not updated, which makes me think that's an unlikely change at this point.

You say d8 views still does the same? That to me is something that should be fixed asap. I'll try and take a look.

drunken monkey’s picture

Status: Needs review » Needs work

To me this looks like either views isn't interfacing a method it should

Let's start earlier, at: "Views doesn't use interfaces." And then it gets worse.
But yeah, I guess since I've already relented in D8, there's no point in making things more difficult in D7. Let's just add that method and be done with it.

For D8, there is already an issue: #2484565: Views is broken with non-Sql query plugins. (The title is a bit of an exaggeration, but it's still pretty annoying, of course.)
No real progress, though, except the consensus that it should be changed, and since D8 is now stable it has probably gotten a bit more complicated to resolve.

For the patch itself:

  1. +++ b/contrib/search_api_views/includes/query.inc
    @@ -180,6 +180,32 @@ class SearchApiViewsQuery extends views_plugin_query {
    +  function add_field($table, $field, $alias = '', $params = array()) {
    

    The documentation for the method should be adapted to meet the Drupal coding standards, to reflect that all arguments except $field are ignored and should probably include an @see to the original method.

  2. +++ b/contrib/search_api_views/search_api_views.views.inc
    @@ -158,6 +158,42 @@ function search_api_views_views_data() {
    + * Implements hook_views_data().
    + */
    +function search_api_views_data_alter(&$data) {
    

    Wrong hook name in doc comment.

Otherwise it looks good, and if you say it makes Search API and VBO run smoothly together, that's good enough for me. Would be great to get some more testers, though.
Also, I guess I'll wait for the VBO issue to be committed before committing this one.

damien_vancouver’s picture

I tested the patch from #75 - with it and the patch from #25 of #1334374: Re-use generic entity views table I was able to remove my custom module code and my search_api and vbo are cooperating.

damien_vancouver’s picture

Status: Needs work » Needs review
FileSize
3.94 KB

I spoke too soon, I was getting an error from the safeguard code:

Notice: Undefined index: search_api_views in search_api_views_module_implements_alter() (line 23 of sites/all/modules/search_api/contrib/search_api_views/search_api_views.module).

(I am using the latest -dev VBO). Once I removed function search_api_views_module_implements_alter then everything worked properly.

I think the safeguard code needs to also check for isset( $implementations['search_api_views']) ?

  function search_api_views_module_implements_alter(&$implementations, $hook) {
  // Ensure our views data alter runs after VBO so we can test for support.
  if ($hook == 'views_data_alter' && isset($implementations['views_bulk_operations']) 
        && isset($implementations['search_api_views'])) {
    $group = $implementations['search_api_views'];
    unset($implementations['search_api_views']);
    $implementations['search_api_views'] = $group;
  }
} 

I've created a new patch with the documentation fixes from #78 and with this extra isset() added which takes care of that error message.

Status: Needs review » Needs work

The last submitted patch, 80: search_api-vbo-1123454-80.patch, failed testing.

damien_vancouver’s picture

Status: Needs work » Needs review
FileSize
3.94 KB

This #82 is the same patch as #80 but without the extra whitespace that failed testing.

Status: Needs review » Needs work

The last submitted patch, 82: search_api-vbo-1123454-82.patch, failed testing.

damien_vancouver’s picture

Status: Needs work » Needs review
FileSize
3.83 KB

#84 (misnamed -86) is the 3rd lucky re-roll I hope.

andrewbelcher’s picture

@damien_vancouver thanks for taking a look. Unfortunately, the actual issue is that the hook was named wrong. This patch resolves that.

drunken monkey’s picture

Thanks for catching that!
I also had some little doc improvements, but otherwise this looks good.

+++ b/contrib/search_api_views/search_api_views.views.inc
@@ -158,6 +158,44 @@ function search_api_views_views_data() {
+      // Check supports generic for entity tables and this entity type.

What is that supposed to mean?

andrewbelcher’s picture

@drunken monkey that's referencing #1334374: Re-use generic entity views table which is switching from using the base table to using the generic views_entity_ENTITY_TYPE table. It's essentially a check that the VBO patch is in as we don't have a tagged version to depend on. Means this could potentially be committed prior to that VBO issue being fixed or having a tagged release...

Also, doc changes look great :)

Arne Slabbinck’s picture

Thx! #86 works like a charm!

drunken monkey’s picture

Great to hear, thanks for testing!
Attached is a patch with clarified phrasing for the inline comments.

Then I guess we just need to wait until the VBO patch is committed. (True, andrewbelcher, we could also commit it before that, due to the proper defensive coding – but we have to make sure, they don't change anything yet before committing that breaks this for us.)

andrewbelcher’s picture

I spoke with bojanz and he seemed to think all the vbo patch needed was tests. I may be able to do that this week...

andrewbelcher’s picture

So... I feel I may need to wear a dunce hat for a while. I think that the work I did over at #1334374: Re-use generic entity views table (which may get committed v soon) actually renders the entirety of this as redundant... The entire point of that patch was to make use of entity information already available by using the generic entity tables (which Search API already correctly joins to). I've just done some testing with Search API and Search API SOLR with just the VBO patch and it all looks like it works properly.

Would appreciate if other people could do some testing to verify that I was and am no longer going mad..? But we may be able to just close this (though the add_field change would still be good for entity reference, but that can be a separate issue)...

Artusamak’s picture

#91 I confirm that you are not mad and that only the patch in #1334374-36: Re-use generic entity views table is enough!

I tested with and without the patch in #89 and it doesn't make a difference if you patch VBO with #1334374-36: Re-use generic entity views table.

drunken monkey’s picture

Then I guess we can just close here?

drunken monkey’s picture

Status: Needs review » Postponed (maintainer needs more info)
pebosi’s picture

It's working for me in combination with https://www.drupal.org/node/1334374#comment-11081609

drunken monkey’s picture

Status: Postponed (maintainer needs more info) » Fixed

OK, thanks!

Status: Fixed » Closed (fixed)

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