Move most or all of the ActionBulkForm class to BulkFormBase

+++ b/core/modules/system/lib/Drupal/system/Plugin/views/field/BulkFormBase.php
@@ -73,6 +73,52 @@ public function preRender(&$values) {
+   * Overrides \Drupal\views\Plugin\views\field\FieldPluginBase::defineOptions().
...
+   * Overrides \Drupal\views\Plugin\views\field\FieldPluginBase::buildOptionsForm().
...
+   * Overrides \Drupal\views\Plugin\views\HandlerBase::validateOptionsForm().

Let's use @inheritdoc here.

+++ b/core/modules/user/lib/Drupal/user/Tests/Views/BulkFormTest.php
@@ -42,7 +42,7 @@ public function testBulkForm() {
-    $this->assertText(t('No users selected.'));
+    $this->assertText(t('No items selected.'));

I am wondering whether the string should be overridden for users.

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/NodeBulkForm.php
index f7c5ea2..08b85b8 100644
--- a/core/modules/system/lib/Drupal/system/Plugin/views/field/BulkFormBase.php

--- a/core/modules/system/lib/Drupal/system/Plugin/views/field/BulkFormBase.php
+++ b/core/modules/system/lib/Drupal/system/Plugin/views/field/BulkFormBase.php

I am not sure whether we talked about that already, but I would like to move this to views out of system. (I am not sure whether you consider this out of scope here).

That is perfect now.

@@ -165,6 +235,35 @@ public function views_form_submit(&$form, &$form_state) {
+   * Set a message text to be displayed when there are no items selected in the
+   * listing.

Can't you make some changes to get it to a one-liner, forget a word or something like that :p

How about "Returns the message to be displayed when there are no selected items."?

@@ -165,6 +235,34 @@ public function views_form_submit(&$form, &$form_state) {
+   * @return string

Even the @return need a description :(

Added an issue summary, so I can RTBC it!

So at the moment this patch does do much because #2020979: Bulk action form no longer allows actions to be selected changed class NodeBulkForm extends ActionBulkForm { :)

The question now becomes should we do this? If so should NodeBulkForm extend BulkFormBase again?

The situation in core has changed a bit since this was filed. Since ActionBulkForm (previously known as BulkForm) is now extended by Node and User modules, we have a new problem that could be resolved with this same solution, so I updated the issue summary to focus on this problem. Out-of-the-box, currently there are *two* options for bulk operations for both nodes and users, one called "Action: Bulk update" and the other called "Content: Bulk update" (or "User: Bulk update"). From the appearance of things, the *only* difference between these is the validation errors are more specific on the Content/User variants.

From an end-user perspective, having two nearly identical fields for the most common data types adds unneeded confusion. If we continue with the solution proposed here of moving/merging BulkForm into BulkFormBase, we could remove BulkForm entirely from hook_views_data() and cut the dependency tree down one level. This would have the effect of requiring other entity types to provide their own classes that extend BulkFormBase like User and Node have done, but I would think that would be a good thing considering some entity types (i.e. Taxonomy terms) don't have any available actions at all (though that may change after #1839516: Introduce entity operation providers, I believe).

This rerolls the last patch as well as adds on action bulk form per entity type. Additional node/user alters the entry in order to set its own plugin ID.

Generally this is looking like a pretty sensible idea. However, can we also do this? and remove the need for basically the same code (in different places) for BulkFormBase and (User/Node bulk forms)?

It would be nice if we didn't need the User and Node bulk forms too, as it's alters, and field handlers - whilst not accomplishing too much.

Just to defend the current state of things (prior to this patch); all of this stuff was written when actions were still just a part of the action module. So I guess now some of it does look a bit crazy :)

Sure that works. Though why inject the whole entity manager and not the action storage?

Why not indeed.

Looks great!

Updated the unit tests.

Great, thanks! Sorry for adding that init() method :p

Thank you for updating the unit test!

How come we've not removed ActionBulkForm? I can't find an usages.

ActionBulkForm is used to generate a bulk action field for all entity types. See views.views.inc

class ActionBulkForm extends BulkFormBase {

}

abstract class BulkFormBase extends FieldPluginBase {
// All the methods...
}

Well ActionBulkForm is an empty class so BulkFormBase is not very abstract then :)

Can't we remove ActionBulkForm and make BulkFormBase concrete and use it in views_views_data() instead of ActionBulkForm?

Here we go. removed ActionBulkForm and renamed BulkFormBase to BulkForm. How about that?

works for me

It works for alex and me

error: patch failed: core/modules/action/tests/action_bulk_test/config/views.view.test_bulk_form.yml:35
error: core/modules/action/tests/action_bulk_test/config/views.view.test_bulk_form.yml: patch does not apply

Here's a reroll.

and back.

Committed dd4890b and pushed to 8.x. Thanks!