Get rid of BulkForm subclasses when they just override emptySelectedMessage

While I agree this could be a good idea, I went and checked, and Comments have a different string (not just the entity label differs).

With the attached patch this would happen:

• Nodes: 'No content selected.'

becomes No content items selected.

• Users: 'No users selected.'

becomes No user entities selected..

• Comments: 'Select one or more comments to perform the update on.'

becomes No comments selected.

So we will have to make a decision on whether the new messages would be good enough...

I also noticed that if I remove NodeBulkForm.php or CommentBulkForm.php then the bulk form fields do not appear on the view, so I have left the empty class there.

We need a usability review here.

I also noticed that if I remove NodeBulkForm.php or CommentBulkForm.php then the bulk form fields do not appear on the view, so I have left the empty class there.

You will need to update \Drupal\node\NodeViewsData to point to the generic bulkform field, for comments too. This would also require an update path to point all existing node/comment bulk fields to the new handler id.

I also noticed that if I remove NodeBulkForm.php or CommentBulkForm.php then the bulk form fields do not appear on the view, so I have left the empty class there.

Can we even remove these classes? Sounds like BC break as mentioned classes are not @internal

Can we even remove these classes? Sounds like BC break as mentioned classes are not @internal

Since they are not marked as anything, it falls into the great big grey 'we will try not to break it' area. Since there is a base class which will most likely be used for extending, and extending these two classes makes zero sense, I think we are fine here with removing them.

Leaving them in makes this patch much easier then taking them out though, we would just need to fix the test fails in #2. I'm not a fan of leaving empty classes in without a good reason, but I'm fine with whatever others want to go with.

Thanks for the input. I believe potential BC break is good enough reason for leaving empty classes in place.

You will need to update \Drupal\node\NodeViewsData to point to the generic bulkform field, for comments too. This would also require an update path to point all existing node/comment bulk fields to the new handler id.

Even if we are not gonna remove these classes just deprecate them, views handler ids must be updated.

So IMO remaining steps are:

• Mark empty classes with @deprecated annotation
• Fix failing tests
• Update views handler ids
• Prepare upgrade path
• Test upgrade path

Removing Novice tag for now.

Thanks all for clarifying the steps needed here.

I feel however that a usability review is what is blocking this issue right now, as it is not clear that we actually want to do this. There may be good reasons for not displaying the messages that would be displayed if we do this (see #2.

To give an example, displaying "No user entities selected." is not going to be very friendly. Perhaps the way forward is to change the plural label of the user entity to be just "users" if we want to do this. The other 2 might be ok, but that's for the usability team to decide.

I am first time sprinter on Vienna2017 DrupalCon and will work on this

Just worked on the patch to avoid test failure

Changing status

@tobiberlin thanks for working on this, it looks like the patch came out wrong, check this page for instructions: https://www.drupal.org/node/707484

@tobiberlin
I also recognized that the patch didn't apply. I just tried to rework on this. I basically didn't change anything, but created the patch with git diff. I hope you are not mad at me ;)

@sfuchsbe is working on this at the sprints

OK patch on #20 looks good... thanks @sfuchsbe

This needs usability review due to the changes in the messages shown to the user (see #2).

The changes proposed do not make a whole lot of sense to me. I am in agreement with jibrans first impression here.

We overall avoid 1) using the word entities in common user interfaces, 2) we try to provide "helpful" messages to get people to act appropriately.

We purposely overwrote these messages to be more helpful - see https://www.drupal.org/docs/develop/user-interface-standards/table and https://www.drupal.org/docs/develop/user-interface-standards/interface-text.

Feel free to tag it again, when a new proposal is made.

How about moving the message to a config setting in the BulkForm plugin? And using that message in \Drupal\system\Plugin\views\field\BulkForm::emptySelectedMessage if it's been set or else fall back to the default message proposed here?
That would not break BC since just overriding emptySelectedMessage would still work too.

But since that would require a schema change, it might have to wait on #2916451: Move everything related to Bulk Form to Views module

Thanks @Bojhan, I agree.

@Lendude that is a great idea, would also be a better developer experience in my opinion. Would that allow us to get rid of BulkForm subclasses as well?

Postponing on #2916451: Move everything related to Bulk Form to Views module for now in any case.

@Manuel Garcia well we'd need an upgrade path that checks if a View uses any of those three plugins and if so:

- update the plugin id to the generic bulk_form
- update the 'empty message' setting to the original message in the old bulk plugin

Then we can deprecate the 3 old plugins (or remove them, but see #8/#9 for arguments against that).

#2916451: Move everything related to Bulk Form to Views module just got in

This would be really helpful for #2877383: Add action support to Media module as well

Rerolled the patch and implemented the proposal from #23

Thanks @chr.fritsch for taking that step =)

+++ b/core/modules/user/src/Tests/Views/BulkFormTest.php
@@ -47,7 +47,7 @@ public function testBulkForm() {
diff --git a/core/modules/views/src/Plugin/views/field/BulkForm.php b/core/modules/views/src/Plugin/views/field/BulkForm.php

diff --git a/core/modules/views/src/Plugin/views/field/BulkForm.php b/core/modules/views/src/Plugin/views/field/BulkForm.php
index a253fc3ad5..f75173757e 100644

index a253fc3ad5..f75173757e 100644
--- a/core/modules/views/src/Plugin/views/field/BulkForm.php

--- a/core/modules/views/src/Plugin/views/field/BulkForm.php
+++ b/core/modules/views/src/Plugin/views/field/BulkForm.php

+++ b/core/modules/views/src/Plugin/views/field/BulkForm.php
+++ b/core/modules/views/src/Plugin/views/field/BulkForm.php
@@ -167,6 +167,10 @@ protected function defineOptions() {

@@ -167,6 +167,10 @@ protected function defineOptions() {
     $options['selected_actions'] = [
       'default' => [],
     ];
+    $options['empty_selected_message'] = [
+      'default' => $this->t('No @entities selected.', ['@entities' => $this->getEntityManager()->getDefinition($this->getEntityType())->getPluralLabel()])
+    ];
+
     return $options;
   }
 
@@ -196,6 +200,12 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {

@@ -196,6 +200,12 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
       '#options' => $this->getBulkOptions(FALSE),
       '#default_value' => $this->options['selected_actions'],
     ];
+    $form['empty_selected_message'] = [
+      '#type' => 'textfield',
+      '#title' => $this->t('Empty selected message'),
+      '#default_value' => $this->options['empty_selected_message'],
+      '#description' => $this->t('The message shown when no items were selected.'),
+    ];
 
     parent::buildOptionsForm($form, $form_state);
   }
@@ -397,7 +407,7 @@ public function viewsFormSubmit(&$form, FormStateInterface $form_state) {

@@ -397,7 +407,7 @@ public function viewsFormSubmit(&$form, FormStateInterface $form_state) {
    *   Message displayed when no items are selected.
    */
   protected function emptySelectedMessage() {
-    return $this->t('No items selected.');
+    return $this->options['empty_selected_message'];
   }

I totally love this idea, gives more power to the site builder to configure these messages.

+++ b/core/modules/comment/tests/src/Functional/CommentAdminTest.php
@@ -102,7 +102,7 @@ public function testApprovalAdminInterface() {
-    $this->assertText(t('Select one or more comments to perform the update on.'));
+    $this->assertText(t('No comments selected.'));

+++ b/core/modules/comment/tests/src/Functional/Views/CommentAdminTest.php
@@ -119,7 +119,7 @@ public function testApprovalAdminInterface() {
-    $this->assertText(t('Select one or more comments to perform the update on.'));
+    $this->assertText(t('No comments selected.'));

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

I believe we should aim to keep the same messages we are currently showing to the user, in order to make this patch not have to deal with string changes. This means we shouldn't have to change the tests verifying that these strings appear. So we'll need an upgrade path to set these options for these three views I think?

Great first steps to getting this done @chr.fritsch!

I believe we should aim to keep the same messages we are currently showing to the user

Yeah absolutely, since that was what was explicitly needed after the usability review.

So I think #28 still needs:

• Update the bulk field schema to add the setting empty_selected_message option
• Update all config for the default Views in core to have the right message set or else the default value
• Write an update path for existing Views to add this setting
• Write a test for the update

The first two steps should mean that all existing tests should come back green with no changes needed. This should be our goal.

Perfect thanks for the clarity @Lendude - I'm gonna give this another push =)

OK some progress in this direction.

I fixed some tests

Yay, tests are green. And here is the update path

@chr.fritsch this looks really nice. Great work on the upgrade path and test!

Couple of things:

1. +++ b/core/modules/views/views.install
   @@ -508,3 +508,36 @@ function views_update_8500() {
   +function views_update_8501() {
   

   Can we turn this into a post_update please (also update the @see in the test)? Less hassle when it gets committed and less change of naming collisions.

2. +++ b/core/modules/views/views.install
   @@ -508,3 +508,36 @@ function views_update_8500() {
   +          if (isset($plugins[$field['plugin_id']])) {
   +            $field['empty_selected_message'] = $plugins[$field['plugin_id']];
   +            $changed = TRUE;
   +          }
   

   Shouldn't we also set the plugin_id to the generic bulk field plugin? Since the entity specific plugins no longer provide any functionality.
   That would also allow us to deprecate the old entity specific plugins. Which I think we should also do here (sorry that was missing from my little todo list in #31).

   Also, when as View is using the generic bulk_form plugin the value should default to something, probably an empty string (also see my next point)

3. +++ b/core/modules/views/src/Plugin/views/field/BulkForm.php
   @@ -167,6 +167,12 @@ protected function defineOptions() {
   +    $options['empty_selected_message'] = [
   +      'default' => $this->t('No @entities selected.', [
   +        '@entities' => $this->getEntityManager()->getDefinition($this->getEntityType())->getPluralLabel(),
   +      ]),
   +    ];
   

   I think the default should be an empty string. And this logic should go into emptySelectedMessage() for when the current value is an empty string (see my previous point). Otherwise we will have a hard time dealing with existing config, since defaults are only set when initialising the View. Also, setting this to an empty string by default would allow the empty message to be automatically updated if the plural label for an entity ever changes.

4. +++ b/core/modules/user/src/Entity/User.php
   @@ -21,6 +21,13 @@
   + *   label_collection = @Translation("User"),
   + *   label_singular = @Translation("user"),
   + *   label_plural = @Translation("users"),
   + *   label_count = @PluralTranslation(
   + *     singular = "@count user",
   + *     plural = "@count users"
   + *   ),
   

   This seems like a change that might need it's own issue, not sure we can get away with just squeezing that in here.

Regarding #38:

38.2: We could do that for node and comment. The user_bulk_form still contains some code.
38.4: This change should be done in #2702683: Add plural labels to entity types which is postponed on #2813895: Combine entity type labels in an array...

#2702683: Add plural labels to entity types landed. So lets proceed.

I rerolled the patch and fixed the comments from #38.

Added the new deprecation warnings to the DeprecationListenerTrait

wow, nice clean up!

1. +++ b/core/modules/comment/src/Plugin/views/field/CommentBulkForm.php
   @@ -2,20 +2,16 @@
   +class CommentBulkForm extends BulkForm {}
   
   +++ b/core/modules/node/src/Plugin/views/field/NodeBulkForm.php
   @@ -2,20 +2,16 @@
   +class NodeBulkForm extends BulkForm {}
   

   Minor: guess, Drupal coding standard would through warning here as "}" should appear in next line?

2. +++ b/core/modules/comment/config/schema/comment.views.schema.yml
   @@ -16,6 +16,8 @@ views.field.comment_entity_link:
    views.field.comment_bulk_form:
      type: views_field_bulk_form
      label: 'Comment bulk form'
   
   +++ b/core/modules/node/config/schema/node.views.schema.yml
   @@ -74,6 +74,8 @@ views.field.node:
   +# @deprecated in Drupal 8.6.x, to be removed before Drupal 9.0.0.
   +# @see https://www.drupal.org/node/xxxx
    views.field.node_bulk_form:
      type: views_field_bulk_form
      label: 'Node bulk form'
   

   If we are removing the body, do we still need to keep schema?

3. +++ b/core/modules/views/views.post_update.php
   @@ -372,3 +372,39 @@ function views_post_update_table_display_cache_max_age(&$sandbox = NULL) {
   +  foreach ($views as $view_name) {
   

   As @catch pointed out in [2880149-124], we might want to use the helper class from #2949351: Add a helper class to make updating configuration simple

Could we also add a change record or update existing (found just https://www.drupal.org/node/2916716) to reflect this approach?

Re #44.2 I believe we can, though not entirely sure, here my reasoning:
The "old" fields are still available, just that now they work differently (using the plugin bulk_form instead), so everything should still work. The only thing we need to deprecate is the views plugin classes I believe.

Taking a stab at that to see how it would look like.

I guess we do need the schema after all... or at least I can't figure out how to make empty_selected_message available to all fields using the bulk_form plugin...

@Manuel Garcia as far as I can tell, it just the

views.field.user_bulk_form:
  type: views_field_bulk_form
  label: 'User operations bulk form'

that needs to stay in, because that plugin is still in use in views.view.user_admin_people

All the schema has to stay because the classes will stay as well. That's also what we did when we deprecated action plugins for example.

Thanks for clarifying things!

So here is another attempt at further cleaning up, only removing the bulk form fields from the views data, and adding the deprecation notice to the views.field.user_bulk_form schema.

OK then, we need those as well. So here's #43 and adding the deprecation notice too views.field.user_bulk_form schema. I'll stop the noise here now sorry :)

So to recap, what needs to happen here as far as I can see:

• Figure out whether we should wait for #2949351: Add a helper class to make updating configuration simple and use that class on views_post_update_set_empty_selected_message().
• Add a change record.

Added a CR https://www.drupal.org/node/2956578

The patch needs a quick update to point to this.

And yes it would be nice to use #2949351: Add a helper class to make updating configuration simple.

Thanks for the CR @Lendude!

Clearly yesterday wasn't my day... we are not deprecating the user bulk form class, so removing the comment added on #53.

Updated the patch to point to the CR, and updated the CR to state the classes that we're deprecating explicitly.

Postponing on #2949351: Add a helper class to make updating configuration simple.

#2949351: Add a helper class to make updating configuration simple landed, so this is unblocked now

So I rerolled that patch and it's now using ConfigEntityUpdater

1. +++ b/core/modules/comment/src/Plugin/views/field/CommentBulkForm.php
   @@ -2,20 +2,16 @@
   +@trigger_error(__NAMESPACE__ . '\CommentBulkForm is deprecated in Drupal 8.6.x, will be removed before Drupal 9.0.0. Use \Drupal\views\Plugin\views\field\BulkForm instead. See https://www.drupal.org/node/xxxx.', E_USER_DEPRECATED);
   ...
   + * @deprecated in Drupal 8.6.x, will be removed before Drupal 9.0.0. Use
   + *   \Drupal\views\Plugin\views\field\BulkForm instead.
   

   still missing the right 'see' to the CR

2. +++ b/core/modules/node/src/Plugin/views/field/NodeBulkForm.php
   @@ -2,20 +2,16 @@
   +@trigger_error(__NAMESPACE__ . '\NodeBulkForm is deprecated in Drupal 8.6.x, will be removed before Drupal 9.0.0. Use \Drupal\views\Plugin\views\field\BulkForm instead. See https://www.drupal.org/node/xxxx.', E_USER_DEPRECATED);
   ...
   + * @deprecated in Drupal 8.6.x, will be removed before Drupal 9.0.0. Use
   + *   \Drupal\views\Plugin\views\field\BulkForm instead.
   

   also missing the correct 'see'

3. +++ b/core/modules/views/views.post_update.php
   @@ -364,3 +364,36 @@ function views_post_update_table_display_cache_max_age(&$sandbox = NULL) {
   +function views_post_update_set_empty_selected_message(&$sandbox = NULL) {
   +
   +  $plugins = [
   +    'user_bulk_form' => 'No users selected.',
   +    'node_bulk_form' => 'No content selected.',
   +    'comment_bulk_form' => 'Select one or more comments to perform the update on.',
   +  ];
   

   Shouldn't we also set a default empty string for views using bulk_form currently?

4. +++ b/core/modules/views/views.post_update.php
   --- a/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
   +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
   
   +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
   @@ -189,6 +189,8 @@ public static function getSkippedDeprecations() {
   +      'Drupal\comment\Plugin\views\field\CommentBulkForm is deprecated in Drupal 8.6.x, will be removed before Drupal 9.0.0. Use \Drupal\views\Plugin\views\field\BulkForm instead. See https://www.drupal.org/node/xxxx.',
   +      'Drupal\node\Plugin\views\field\NodeBulkForm is deprecated in Drupal 8.6.x, will be removed before Drupal 9.0.0. Use \Drupal\views\Plugin\views\field\BulkForm instead. See https://www.drupal.org/node/xxxx.',
   

   Shame these need to be added :( But no way around this since I guess plugin discovery is triggering this?

Fixed everything

+++ b/core/modules/views/views.post_update.php
@@ -374,6 +374,7 @@ function views_post_update_set_empty_selected_message(&$sandbox = NULL) {
+    'bulk_form' => '',

Looks like this broke our post_update function :S

Rerolled, had to manually fix these due to #2935256: Remove all usages of drupal_get_message and drupal_set_message in modules:

• core/modules/comment/tests/src/Unit/Plugin/views/field/CommentBulkFormTest.php
• core/modules/node/tests/src/Unit/Plugin/views/field/NodeBulkFormTest.php

Had a go at the failing test, but I cant figure it out... views_post_update_set_empty_selected_message() looks sound to me, and so does the test...

For now just adding coverage for the second display 'page_unapproved' on the comment view, which also uses the comment_bulk_form field.

I looked into the failing test and found something sad :(

In #1986606: Convert the comments administration screen to a view the comment list was moved to be a list. But it uses the current config from core/modules/comment/config/optional/views.view.comment.yml to create the view. In views_post_update_set_empty_selected_message we expect to have a comment view that still uses the comment_bulk_form plugin. But in the testViewsEmptySelectedMessageUpdate it doesn't. It already uses the bulk_form. Because that's the new value in views.view.comment.yml. So our views_post_update_set_empty_selected_message resets empty_selected_message to an empty string and that's not what we expect for the comment view.

Maybe we should open an issue to adjust the comment_post_update_enable_comment_admin_view function. It better should use the config from a year ago and not the current values.

Reroll, manually fixed conflict on core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php, and changed the deprecations to mention 8.7.x instead of 8.6.x.

Thanks a lot for the discovery on the failing test @chr.fritsch - I now understand the problem. I don't see how we can adjust comment_post_update_enable_comment_admin_view() to use a previous version of the view though.

However I think we can work around this in our post_update function, please have a look :)

Humm are those deprecation notices because of the patch?

We needed to add @group legacy to our update path test, see https://www.drupal.org/node/2985785

As much as those classes look silly, I don't think this is a good idea.

We're now violating DRY because this phrase is repeated:

+++ b/core/modules/comment/config/optional/views.view.comment.yml
@@ -182,7 +182,9 @@ display:
+            'Select one or more comments to perform the update on.'
+          plugin_id: bulk_form
           entity_type: comment
         subject:
           id: subject
@@ -1119,7 +1121,9 @@ display:

@@ -1119,7 +1121,9 @@ display:
           selected_actions:
             - comment_delete_action
             - comment_publish_action
-          plugin_id: comment_bulk_form
+          empty_selected_message:
+            'Select one or more comments to perform the update on.'

More importantly though, this change deteriorates UX.

Before, I set up a comment view, and the empty select message is there for me, OOTB.

After, I have to write it myself for each view I make, or have a default value that's generic to all entity types.

Before, I set up a comment view, and the empty select message is there for me, OOTB.

After, I have to write it myself for each view I make, or have a default value that's generic to all entity types.

I don't agree that this deteriorates UX, you still get an empty selected message out of the box, being No comments selected for a comments view for example. You only would need to configure it if you wanted something custom for that specific view.

In fact, in my opinion this actually improves the UX, since this gives more power to the site builders who would now be able to configure the empty selection message via the UI, whereas before they couldn't.

Updated the IS to reflect more the reasons and advantages of doing this, current status of the issue, and to mention deprecation of two classes.

Reroll of #76, fixed conflicts manually on:

• core/modules/views/views.post_update.php
• core/modules/node/tests/src/Unit/Plugin/views/field/NodeBulkFormTest.php
• core/modules/comment/tests/src/Unit/Plugin/views/field/CommentBulkFormTest.php

Conflicts on the other test classes made me have a look at our changes to UserBulkFormTest, and indeed it needed updated because of the change in variable name.

Test failed because entity.manager service got deprecated. Took the opportunity to clean it up while I was fixing it.

OK this should hopefully come back green.

Actually there was no need to touch core/modules/user/tests/src/Functional/Views/BulkFormTest.php so removing those changes in this patch.

Sigh, removed changes from the wrong file, here is the correct patch. And this is a good example why you shouldnt be messing about on a Sunday evening instead of resting.

Here is the interdiff from #84 to #90, please ignore the stuff in between.

Sigh, got the interdiff the wrong way around... here is what actually happened:

diff --git a/core/modules/views/src/Plugin/views/field/BulkForm.php b/core/modules/views/src/Plugin/views/field/BulkForm.php
index b9cb6ca..7e35796 100644
--- a/core/modules/views/src/Plugin/views/field/BulkForm.php
+++ b/core/modules/views/src/Plugin/views/field/BulkForm.php
@@ -462,7 +462,7 @@ public function viewsFormSubmit(&$form, FormStateInterface $form_state) {
   protected function emptySelectedMessage() {
     if (empty($this->options['empty_selected_message'])) {
       return $this->t('No @entities selected.', [
-        '@entities' => $this->getEntityManager()->getDefinition($this->getEntityType())->getPluralLabel(),
+        '@entities' => $this->getEntityTypeManager()->getDefinition($this->getEntityType())->getPluralLabel(),
       ]);
     }
     return $this->options['empty_selected_message'];

Apologies for the noise.

Manually tested the patch in #90 and everything looks good. +1 to RTBC.

Result:
1. Upgrade path ran successfully(screenshot)
2. Without and with patch has the same message and behaviour all 3 affected bulk forms: (with patch screenshot, without patch screenshot).

1. +++ b/core/modules/comment/config/optional/views.view.comment.yml
   @@ -182,7 +182,9 @@ display:
   +          empty_selected_message:
   +            'Select one or more comments to perform the update on.'
   
   @@ -1119,7 +1121,9 @@ display:
   +          empty_selected_message:
   +            'Select one or more comments to perform the update on.'
   

   why this strings are not one line?

2. +++ b/core/modules/views/views.post_update.php
   @@ -381,3 +381,47 @@ function views_post_update_exposed_filter_blocks_label_display(&$sandbox = NULL)
   +          if (isset($plugins[$field['plugin_id']])) {
   +            $display['display_options']['fields'][$id]['empty_selected_message'] = $plugins[$field['plugin_id']];
   

   why there's no check that plugin_id is a one from list?

Thanks for reviewing!

Re #97.1 Dont see why it should be like that, so changed them to being on the same line.
#97.2 isset($plugins[$field['plugin_id']]) is checking the list, but just in case I added another check to explicitly check the $id as well.

Looks like the comments in #97 have been addressed and the issue summary is up to date with a change record.

Rerrolled, simple conflict on core/modules/views/views.post_update.php

Random failure in media oembed

+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -127,6 +127,8 @@ public static function getSkippedDeprecations() {
+      'Drupal\comment\Plugin\views\field\CommentBulkForm is deprecated in Drupal 8.7.x, will be removed before Drupal 9.0.0. Use \Drupal\views\Plugin\views\field\BulkForm instead. See https://www.drupal.org/node/2956578.',
+      'Drupal\node\Plugin\views\field\NodeBulkForm is deprecated in Drupal 8.7.x, will be removed before Drupal 9.0.0. Use \Drupal\views\Plugin\views\field\BulkForm instead. See https://www.drupal.org/node/2956578.',

We shouldn't be adding to this list. How come we need to? Update tests ignore legacy deprecations so this is not needed for them.

I'm guessing it is because we are deprecating plugins. We do that in the constructor to avoid plugin scanning triggering deprecations.

Thanks @alexpott - addressing it here.

https://www.drupal.org/core/deprecation#how-plugin

Let's see how many things will be fixed, I think views_data should use new definition as well

Let's see how it affects tests

Thanks @andypost!

#105 is addressed, so I'm tempted to go back to RTBC, but now I'm wondering whether the change to views_data needs an upgrade path or not?

After refreshing my memory reading the upgrade path, we're already handling the change to existing views, and don't see how the change to views_data would require anything extra, so setting this back to RTBC.

+++ b/core/modules/views/views.post_update.php
@@ -389,3 +389,46 @@ function views_post_update_make_placeholders_translatable() {
+
+/**
+* Set empty selected message for bulk operation fields.
+ */
+function views_post_update_set_empty_selected_message(&$sandbox = NULL) {
+  $plugins = [

This doesn't cover the case of modules with default configuration.

What we've done in the past, is to add the logic on configuration save, and just loop over and save the live config in the update without any additional logic.

See views_view_presave() for some examples.

Thanks @catch for reviewing, you're right. Hopefully this is what you meant on #113.

I think the example in \Drupal\views\Entity\View::preSave is better, using \Drupal\views\Entity\View::fixTableNames which is easier to mark as @depricated and make it private, so its much clearer we are dealing with temp code that needs to be removed.

Using views_view_presave makes that hard to do.

@catch are we sure it's worth it? The old plugins still work they are just deprecated. In the case of \Drupal\views\Entity\View::fixTableNames() we had something to fix to make it actually work so I think that's a different case.

Needs work to update deprecations to new format and

+++ b/core/modules/views/src/Plugin/views/field/BulkForm.php
@@ -220,6 +220,10 @@ protected function defineOptions() {
+    $options['empty_selected_message'] = [
+      'default' => '',

maybe provide here default?

Straight re-roll of #114 plus switching to using createMock instead of getMock on the unit tests changes.

Fixing the reroll and adding a default to the option, it was requested on #119 and I think it's a good idea :)

Tackling tests failures.

Tackling one of the failing test, not sure why the other one fails, I tried the upgrade path manually and the empty selected text is saved to the view.

Also moving from views_view_presave to Views::preSave() in case there-s consensus that this is actually needed (see #116 & #117).

Thanks @dawehner for the review, all good points, addressing them here.

This needs to be updated to 8.8.x or even 8.9.x

IIRC we should not deprecate it for 8.9, instead it should be done in 9.0 for 10.0 removal

Patch does 2 things
- fix deprecation message to format https://www.drupal.org/core/deprecation#how-plugin and moved to 9.0.0
- remove fixTableNames() as #128.2 - it's a views definition for entity to provide empty message

PS: probably this method also needed to ecplude deprecated plugins from views plugin discovery!

This would be a minor-only change. Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. Thanks!

+++ b/core/modules/comment/config/schema/comment.views.schema.yml
@@ -16,6 +16,8 @@ views.field.comment_entity_link:
+# @deprecated in Drupal 8.9.x, to be removed before Drupal 9.0.0.

+++ b/core/modules/comment/src/Plugin/views/field/CommentBulkForm.php
@@ -2,20 +2,30 @@
+ * @deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Use
...
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, LanguageManagerInterface $language_manager, MessengerInterface $messenger, EntityRepositoryInterface $entity_repository = NULL) {
+    @trigger_error(__CLASS__ . ' is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Use \Drupal\views\Plugin\views\field\BulkForm instead. See https://www.drupal.org/node/2956578', E_USER_DEPRECATED);

+++ b/core/modules/node/config/schema/node.views.schema.yml
@@ -74,6 +74,8 @@ views.field.node:
+# @deprecated in Drupal 8.9.x, to be removed before Drupal 9.0.0.
+# @see https://www.drupal.org/node/2956578

+++ b/core/modules/node/src/Plugin/views/field/NodeBulkForm.php
@@ -2,20 +2,30 @@
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, LanguageManagerInterface $language_manager, MessengerInterface $messenger, EntityRepositoryInterface $entity_repository = NULL) {
+    @trigger_error(__CLASS__ . ' is deprecated in drupal:9.0.0 and is removed before drupal:10.0.0. Use \Drupal\views\Plugin\views\field\BulkForm instead. See https://www.drupal.org/node/2956578', E_USER_DEPRECATED);

So it should be 9.1 and https://www.drupal.org/core/deprecation#how-class

Re-roll for D9.1.x

@jofitz can you add interdiff to your patch?
It's not clean why the patch size is twice bigger then previous one

The re-roll incomplete - the version it should use is 9.1 and 9.0 in freeze

1. +++ b/core/modules/comment/config/schema/comment.views.schema.yml
   @@ -16,6 +16,8 @@ views.field.comment_entity_link:
   +# @deprecated in Drupal 8.9.x, to be removed before Drupal 9.0.0.
   +# @see https://www.drupal.org/node/2956578
   
   +++ b/core/modules/comment/src/Plugin/views/field/CommentBulkForm.php
   @@ -2,20 +2,30 @@
   + * @deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Use
   + *   \Drupal\views\Plugin\views\field\BulkForm instead.
   ...
   +    @trigger_error(__CLASS__ . ' is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Use \Drupal\views\Plugin\views\field\BulkForm instead. See https://www.drupal.org/node/2956578', E_USER_DEPRECATED);
   
   +++ b/core/modules/node/config/schema/node.views.schema.yml
   @@ -74,6 +74,8 @@ views.field.node:
   +# @deprecated in Drupal 8.9.x, to be removed before Drupal 9.0.0.
   +# @see https://www.drupal.org/node/2956578
   
   +++ b/core/modules/node/src/Plugin/views/field/NodeBulkForm.php
   @@ -2,20 +2,30 @@
   + * @deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. Use
   ...
   +    @trigger_error(__CLASS__ . ' is deprecated in drupal:9.0.0 and is removed before drupal:10.0.0. Use \Drupal\views\Plugin\views\field\BulkForm instead. See https://www.drupal.org/node/2956578', E_USER_DEPRECATED);
   

   now it should be drupal:9.1.0 and 10.0.0

2. +++ b/core/modules/views/views.post_update.php.orig
   @@ -0,0 +1,46 @@
   +function views_removed_post_updates() {
   +  return [
   +    'views_post_update_update_cacheability_metadata' => '9.0.0',
   +    'views_post_update_cleanup_duplicate_views_data' => '9.0.0',
   +    'views_post_update_field_formatter_dependencies' => '9.0.0',
   +    'views_post_update_taxonomy_index_tid' => '9.0.0',
   +    'views_post_update_serializer_dependencies' => '9.0.0',
   +    'views_post_update_boolean_filter_values' => '9.0.0',
   +    'views_post_update_grouped_filters' => '9.0.0',
   +    'views_post_update_revision_metadata_fields' => '9.0.0',
   +    'views_post_update_entity_link_url' => '9.0.0',
   +    'views_post_update_bulk_field_moved' => '9.0.0',
   +    'views_post_update_filter_placeholder_text' => '9.0.0',
   +    'views_post_update_views_data_table_dependencies' => '9.0.0',
   +    'views_post_update_table_display_cache_max_age' => '9.0.0',
   +    'views_post_update_exposed_filter_blocks_label_display' => '9.0.0',
   +    'views_post_update_make_placeholders_translatable' => '9.0.0',
   +    'views_post_update_limit_operator_defaults' => '9.0.0',
   +    'views_post_update_remove_core_key' => '9.0.0',
   

   not sure it needed in 9.1

Looks like we still need to reroll #134, so re-adding the tag.

Trying to fix failed tests

Proper re-roll and fix few nits

Another reroll

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.