Bulk Form field for SAPI

Very nice work!

1. --- a/search_api.views.inc
   +++ b/search_api.views.inc
   @@ -620,6 +620,15 @@ function _search_api_views_data_special_fields(array &$table, IndexInterface $in
   +  $table[$bulk_form_field] = [
   +    'title' => t('Bulk update'),
   +    'help' => t('Allows users to apply an action to one or more items.'),
   +  ];
   

   I would call this the same as the original field: Views bulk operations instead of Bulk update. It is more consistent and this can do more than just update entities (e.g. delete them).

2. --- /dev/null
   +++ b/src/Plugin/views/field/SearchApiBulkForm.php
   +  public function getEntityType() {
   +    // Override the parent method as BulkForm::init() will call this and will
   +    // complain that a valid entity type cannot retrieved.
   +    // @see \Drupal\views\Plugin\views\field\BulkForm::init()
   +    // @see \Drupal\views\Plugin\views\HandlerBase::getEntityType()
   +    return NULL;
   +  }
   

   This explains well why it is overridden, but to make it even more clear to future maintainers it would be good to also explain that this is because Search API views support multiple entity types, like is explained so well in the issue summary. How about something like this:

   +  public function getEntityType() {
   +    // The standard BulkForm only works with a single entity type, but results
   +    // returned by Search API might contain entities of many different entity
   +    // types, and even external data sources that are not based on entities.
   +    // Override the parent method as BulkForm::init() will call this and will
   +    // complain that a valid entity type cannot retrieved.
   +    // @see \Drupal\views\Plugin\views\field\BulkForm::init()
   +    // @see \Drupal\views\Plugin\views\HandlerBase::getEntityType()
   +    return NULL;
   +  }
   

3.     // As the view might contain rows from diverse entity types and an action
       // is designed to act only on a specific entity type, we remove the
       // incompatible selected rows from the selection and popup a warning.
       // @todo Use Javascript in order to disable the checkboxes from the rows
       //   incompatible with the selected action.
   

   I think what is described in this @todo has some UX implications and needs some careful thought. Disabling the checkboxes depending on the chosen action would mean that the user always needs to choose the action first, and after this select the entities from the list. Some users might prefer to first select the entities and secondly the actions. Depending on the default action and the entities that are first in the list it might be the case that none of the entities are actionable (and are disabled) until the user chooses a different action. This can be quite confusing for the end user.

   I think there are definitely good solutions for this: for example we could make it a 2 step operation: first the user chooses the action, and after this we will load the list of actionable entities using AJAX. But this is complex and not blocking for this so I would propose to handle it in a followup issue.

   I found the current workflow to work reasonably well: if the user selects any entities that are not actionable then they will be removed from the list and a warning is shown.

Leaving this assigned to me for now, I still need to review the test coverage.

Finished the review and did manual testing + stepped through the test. The bulk operations view works flawlessly, very impressive work!

Some more remarks that would be good to address:

1. --- /dev/null
   +++ b/tests/search_api_test_bulk_form/src/TypedData/FooDataDefinition.php
   @@ -0,0 +1,21 @@
   +  /**
   +   * @inheritDoc
   +   */
   

   This is using a different standard for inheritdoc, Drupal uses {@inheritdoc}.

2. --- /dev/null
   +++ b/tests/search_api_test_bulk_form/src/Plugin/Action/SearchApiTestBulkFormTestTrait.php
   @@ -0,0 +1,31 @@
   +  public function execute($entity = NULL) {
   +   // ...
   +    $result = $state->get('serach_api_test_bulk_form', []);
   +    // ...
   +    $state->set('serach_api_test_bulk_form', $result);
   +  }
   

   This has a typo in the state key: serach_api_... should be seach_api_....

3. +  protected function calculateEntityBulkFormKey(EntityInterface $entity, $use_revision) {
   +    $bulk_form_key = \json_decode(base64_decode(parent::calculateEntityBulkFormKey($entity, $use_revision)));
   +    // Rows of Search API views, based on entity data sources, might have
   +    // different entity types. We add the entity type ID to the bulk form key.
   +    array_unshift($bulk_form_key, $entity->getEntityTypeId());
   +    $key = json_encode($bulk_form_key);
   +    return base64_encode($key);
   +  }
   

   The call to \json_decode() is prefixed by a backslash so it will always be using the native implementation from the PHP JSON extension which lives in the global namespace. This is usually only necessary when a function with the same name is defined in the current namespace. It is best to avoid this unless needed, since developers will not be able to override the json_decode() function with their own implementation if they need it for whatever reason.

4. +  /**
   +   * Selects a Views row containing a given text.
   +   *
   +   * @param $text
   +   *   Text contained in the row to be selected.
   +   * ...
   +   */
   +  protected function selectRow($text) {
   

   It's not 100% clear what is meant with "Selects a Views row", maybe change it to "Checks the checkbox in the Views row containing the given text." and rename the method to checkCheckboxInRow($text)?

   Also provide the type of the parameter: @param string $text.

5. +    // Check that only entity-bases data source rows have checkboxes. The view
   +    // is sorted by item_id, so the following assertions are safe.
   +    $assert->fieldExists('search_api_bulk_form[0]');
   +    $assert->fieldExists('search_api_bulk_form[1]');
   +    $assert->fieldExists('search_api_bulk_form[2]');
   +    $assert->fieldExists('search_api_bulk_form[3]');
   +    $assert->fieldNotExists('search_api_bulk_form[4]');
   +    $assert->fieldNotExists('search_api_bulk_form[5]');
   

   In this part of the test we are checking that the checkboxes are not shown for the non-entity data sources, but I would also add a test that the rows for these data sources are still visible. For example by checking if their labels are visible, and that the number of visible row is 7 (1 header + 6 result rows).

Looking good now, thanks!

Wow, thanks a lot for this amazing work! And sorry for the long delay in replying!
The code looks almost perfect, and there is even great test coverage. I’m amazed – again, thank you so much!
Thanks a lot for the thorough review, pfrenssen, and helping to give this the last polish. Also great job!

As I’m a bit of a pedant when it comes to coding standards, I do still have some improvements to suggest – please see/test/review the attached revision. But only minor nitpicks, really.
If these changes look OK to you, I’ll commit this.

I reviewed the changes made in #11 and #13 and all is looking good. There was apparently still a lot of polish possible in #11, and the remarks in #12 are correct. The mixing of API calls inside the test + further API calls in requests made during the test can cause $this->container to contain stale data. This is something we have encountered on several occasions already. Thanks @claudiu.cristea for linking that comment, it is explaining it clearly.

This looks very good to me, thanks all!

I have to disagree with these changes. In functional tests $this->container might get out-of-sync. It's always recommended to use \Drupal::service(...) in functional tests as opposed to Kernel tests, where $this->container is safe. See the @alexpott's relevant comment, here #2066993-57: Use magic methods to sync container property to \Drupal::getContainer in functional tests

Thanks a lot for clarifying that!
Seems rather bad design, but now that you mention it I think I remember hearing something like this before. Should probably try to remember it this time … (I also opened #3125444: Get rid of $this->container in Functional tests to take care of the existing instances of it in this module.)

Thanks also, pfrenssen, for reviewing again and setting to RTBC!

Personally, I just prefer \Drupal::getContainer()->get(…) to \Drupal::service(…), as PhpStorm is able to do type inference on the former, but not the latter. So, changed it to that and then committed.

Thanks a lot again, both of you!