Bulk operation actions might not act on the selected items

Wow we should use IDs/uuids or something like that in the form.

BUTBUT we use the entity ID, langcode and revision key:

/**
   * Calculates a bulk form key.
   *
   * This generates a key that is used as the checkbox return value when
   * submitting a bulk form. This key allows the entity for the row to be loaded
   * totally independently of the executed view row.
   *
   * @param \Drupal\Core\Entity\EntityInterface $entity
   *   The entity to calculate a bulk form key for.
   * @param bool $use_revision
   *   Whether the revision id should be added to the bulk form key. This should
   *   be set to TRUE only if the view is listing entity revisions.
   *
   * @return string
   *   The bulk form key representing the entity's id, language and revision (if
   *   applicable) as one string.
   *
   * @see self::loadEntityFromBulkFormKey()
   */
  protected function calculateEntityBulkFormKey(EntityInterface $entity, $use_revision) {
    $key_parts = [$entity->language()->getId(), $entity->id()];

    if ($entity instanceof RevisionableInterface && $use_revision) {
      $key_parts[] = $entity->getRevisionId();
    }

    // An entity ID could be an arbitrary string (although they are typically
    // numeric). JSON then Base64 encoding ensures the bulk_form_key is
    // safe to use in HTML, and that the key parts can be retrieved.
    $key = json_encode($key_parts);
    return base64_encode($key);
  }

We are NOT using its relative position in the form at all.

I can reproduce, and I also am as confused as @Gábor Hojtsy.
Looking into this.

But in BulkForm::viewsForm() the way it is used:

        $form[$this->options['id']][$row_index] = array(
          '#type' => 'checkbox',
          // We are not able to determine a main "title" for each row, so we can
          // only output a generic label.
          '#title' => $this->t('Update this item'),
          '#title_display' => 'invisible',
          '#default_value' => !empty($form_state->getValue($this->options['id'])[$row_index]) ? 1 : NULL,
          '#return_value' => $this->calculateEntityBulkFormKey($entity, $use_revision),
        );

"#default_value" is filled in from $form_state using "$row_index", so when new nodes are added, and the "Apply to selected items" is pressed, form is rebuilt and the new nodes "take place" of the old ones, because default sort is by "Updated" column.

Why in gods name are we JSON encoding and base64 encoding the form keys again?!

The encoding is from #2599246: data in bulk_form_key should not be separated by -.

Thanks Tim! It's bringing back memories now.

@tacituseu Great find!

Fix combined with test from #11

And again...

Won't be so easy, it goes all the way down to Drupal\Core\Render\Element\Checkbox::valueCallback():

public static function valueCallback(&$element, $input, FormStateInterface $form_state) {
  if ($input === FALSE) {
    // Use #default_value as the default value of a checkbox, except change
    // NULL to 0, because FormBuilder::handleInputElement() would otherwise
    // replace NULL with empty string, but an empty string is a potentially
    // valid value for a checked checkbox.
    return isset($element['#default_value']) ? $element['#default_value'] : 0;
  }
  else {
    // Checked checkboxes are submitted with a value (possibly '0' or ''):
    // http://www.w3.org/TR/html401/interact/forms.html#successful-controls.
    // For checked checkboxes, browsers submit the string version of
    // #return_value, but we return the original #return_value. For unchecked
    // checkboxes, browsers submit nothing at all, but
    // FormBuilder::handleInputElement() detects this, and calls this
    // function with $input=NULL. Returning NULL from a value callback means
    // to use the default value, which is not what is wanted when an unchecked
    // checkbox is submitted, so we use integer 0 as the value indicating an
    // unchecked checkbox. Therefore, modules must not use integer 0 as a
    // #return_value, as doing so results in the checkbox always being treated
    // as unchecked. The string '0' is allowed for #return_value. The most
    // common use-case for setting #return_value to either 0 or '0' is for the
    // first option within a 0-indexed array of checkboxes, and for this,
    // \Drupal\Core\Render\Element\Checkboxes::processCheckboxes() uses the
    // string rather than the integer.
    return isset($input) ? $element['#return_value'] : 0;
  }
}

Here, $input holds the proper (old) value from $_POST but the $element is for the "newly added" node and has its $bulk_form_key, reason is:

$form[$this->options['id']][$row_index] = array(...)

the old form and rebuilt one have same id for first checkbox, but the rebuilt one has "newly added" node in there, to fix it $bulk_form_key would have to be part of the id, something like:

$form[$this->options['id']][$bulk_form_key] = array(...)

but it won't work since views rely on it being indexed by row number, so maybe something like:

$form[$this->options['id']][$row_index][$bulk_form_key] = array(...)

Even if proper/unique id would fix the "deleting wrong node" part of the problem, the other part of it is that nodes selected at the very bottom of the page would not get chosen action applied.

This should work as a data loss fix, doesn't use raw user input, instead plays with '#parents', but still doesn't cover #17.

The failures are from tests assuming, ['node_bulk_form[0]' => TRUE] would check the first checkbox, like in:
$this->drupalPostForm(NULL, ['node_bulk_form[0]' => TRUE], 'Apply to selected items');
which it no longer does, but the code works.

Fixing tests by xpath'ing out the proper names/ids for checkboxes.

@swentel: this was just to preserve logic that was already there, the magic is in '#parents'.

New patch, with fewer errors, hopefully.

Random segfault in Drupal\node\Tests\Views\FrontPageTest.

Please also take a look at an issue I opened (#2845634). It's definitely a related issue, but I'm not sure if it's the exact same issue.

I experienced this same issue in a different way. I found that using AJAX to filter or sort a table was enough to cause the index to lose integrity.

The difference between this report and mine (#2845634) is that in this one, the index loses integrity when new content is added while a user is in the middle of performing a bulk action. In my report, no new content needs to be introduced. If AJAX sorting or filtering is used, the index will lose integrity and the actions will be performed on the wrong nodes.

When I did experience this behavior, the following conditions were true:

• AJAX sorting/filtering is enabled for the Display
• The Display is using Node Bulk Operations
• The Display is embedded into a page
• Either a sort or a filter action is performed on the field data being displayed

The exact same view, will work as expected if AJAX is turned off.

Unfortunately, I'm having difficulty reproducing this on a clean install. I'm mentioning it here in case it helps identify the root cause here.

If anyone thinks it would be valuable for me to try to pin down exactly how to reproduce the issue I described in #2845634, let me know. I'll see if I can identify the exact factors.

@hyperlinked (#38): I noticed some AJAX related issues in Chrome related to going back in history, it seems to go back to base non-ajax html and checkboxes get mixed-up due to having the same id as ones updated by AJAX, to replicate:
0. Make sure the latest node is 'Unpublished', and all the other 'Published'
1. Open 'admin/content'
2. Filter out the top node (e.g. filter for 'Published' only)
3. Select first two nodes and click 'Apply to selected items'
4. All is as expected on confirmation page
5. Go back in browser history to previous page
6. Observe the form being reset to the before AJAX state and the 'Unpublished' node being now selected

@swentel (#39): Yes, I consider my patch to be only a band-aid, i.e. it will prevent you from shooting yourself in the wrong foot. If anyone it's me that hijacked, my approach will get me no further anyway.
Maybe using temp storage and rebuilding the form from it (like you mentioned in #26) could help, but I'm not familiar enough with views module inner workings to take it further, especially with AJAX in the mix.
I feel though that making sure each checkbox has unique id/name is essential part of the solution.
I also worked on extending test coverage to also cover deletion and node from the next page getting into selection (took slightly different approach - attached for comparison).

@alexpott, @xjm, @effulgentsia, @catch and I discussed this issue and agreed this is clearly a critical bug because of the demonstrated data loss.

On its way out though #1986606-326: Convert the comments administration screen to a view (3rd point).

@swentel and I discussed this issue and the fix at the global sprint weekend, I had to let this simmer for a while to fully understand the reason why #40 is a good way to fix this, but I feel confident that patch is correct.
It's small, self-contained and has sufficient test coverage.

Maybe we could add docs in the ::viewsFormSubmit why we're using getUserInput but I don't think that's needed really, the test should be enough to not shoot ourselves in the feet again.

@tacituseu: if the comments view is going to change, it will break as well. So we should probably add a test similar to the one added here for nodes for comments as well (in a seperate issue as a follow-up to this one or in the other issue).
That way we know the behavior happening here won't happen for comments as well.

Anyway, I'm going to rtbc #40 based on that.

You could help it to look a lot less scary by reinforcing BulkForm::loadEntityFromBulkFormKey() a little, something like:

    // If there are 3 items, vid will be last.
    if (count($key_parts) === 3) {
      $revision_id = (int) array_pop($key_parts);
    }

    // The first two items will always be langcode and ID.
    $id = (int) array_pop($key_parts);
    $langcode = substr(array_pop($key_parts), 0, 12);
    if (!preg_match('@^[a-zA-Z]{1,8}(-[a-zA-Z0-9]{1,8})*$@', $langcode)) return NULL;

would prevent it from trusting database drivers too much :)

Oh, #40 is so beautifully simple!

In the future, please always reupload the patch intended for commit. While hiding files helps make the issue summary focus on the right patch, the last patch on the issue will be automatically retested when the issue is RTBC, but other patches won't. Committers can also get confused when reading issue comments from the bottom up as we often do. :) So it's a good way to avoid regressions or mistakes.

@hyperlinked, can you test your issue against the latest HEAD now that this issue is fixed, and post on #2845634: Node Bulk Operation Performs Operations on Wrong Nodes when AJAX Filtering/Sorting is Used whether it also resolves your issue? It did not at first look like it would, but hopefully you can follow up now that this one is fixed.

I do agree with adding inline documentation as @borisson_ suggested. Since this is a data loss critical, I added some inline docs on commit rather than further delay the issue. If my added docs are incorrect or need improvement, please file a followup for fixing them:

diff --git a/core/modules/system/src/Plugin/views/field/BulkForm.php b/core/modules/system/src/Plugin/views/field/BulkForm.php
index a2cfdd2583..8e38c2b732 100644
--- a/core/modules/system/src/Plugin/views/field/BulkForm.php
+++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
@@ -344,7 +344,9 @@ protected function getBulkOptions($filtered = TRUE) {
    */
   public function viewsFormSubmit(&$form, FormStateInterface $form_state) {
     if ($form_state->get('step') == 'views_form_views_form') {
-      // Filter only selected checkboxes.
+      // Filter only selected checkboxes. Use the actual user input rather than
+      // the raw form values array, since the site data may change before the
+      // bulk form is submitted, which can lead to data loss.
       $user_input = $form_state->getUserInput();
       $selected = array_filter($user_input[$this->options['id']]);
       $entities = array();

Similarly, for exploring more complete solutions to improve this generally (as discussed elsewhere on the issue), let's file a followup for that too. That way we can save some data between now and when we agree on other ideas. :)

Committed to 8.4.x and 8.3.x Thanks for fixing this so quickly!

@xjm, I've tested the 8.3 HEAD against my build and it resolved the issue that I reported in #2845634. It was the same issue.

Thanks @hyperlinked!

Oops yeah @swentel, I missed #41. Followup sounds good.

I'm using Drupal 8.9.20 and this issue still happens.

Any solution?