As discussed in several issues it would be really helpful to have something like VBO in core, but in a really light weight version.

It should for now not provide any abstraction over actions vs. rules etc.

#1823574: [Meta] Improve the Views Bulk Operations (VBOs) that are in core

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

This is the absolute simplest version you can do.

  • It doesn't have batch/aggregation support
  • It doesn't have support form configurable actions
dawehner’s picture

Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.x-dev
Component: Code » action.module
Status: Active » Needs review
FileSize
11.39 KB

It's working fine so far, and is relative simple

Status: Needs review » Needs work

The last submitted patch, drupal-1828410-2.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
11.91 KB

Let's fix the test.

Status: Needs review » Needs work

The last submitted patch, drupal-1828410-4.patch, failed testing.

yoroy’s picture

Is there UI in this patch or just plumbing? Thanks.

dawehner’s picture

Status: Needs work » Needs review

#4: drupal-1828410-4.patch queued for re-testing.

dawehner’s picture

Status: Needs review » Needs work
FileSize
23.79 KB
10.96 KB

I'm not that used to core development yet, so here come some screenshots.
As written before, this is the most simplest approach you can have so far.

The first screen shows the actual adding of the views-field, but you don't have any additional
configuration on that yet.
adding.png

On the output it looks like that. One particular helpful feature would be to provide a table select all checkbox.
bulk_form.png

sun’s picture

FileSize
41.3 KB

Just to provide a comparison — here's what VBO looks like in D7:

vbo-d7.png

So that is looking very similar.

Differences / What I'm missing:

- The tableselect/select-all checkbox.

- The form button arguably makes more sense right next to the bulk-update action selector. We should also consider to output the button both at the top and at the bottom.

yoroy’s picture

Nice! And agreed with sun:

- A select all checkbox would def. be a big win.
- Having the 'Apply' (not Execute) button both at top and bottom makes sense here as well
- I'd like to tweak the description for this on the 'Add' screen, something like: 'Add a form element that lets you apply actions to multiple items'

I think continuing with adding the select all checkbox is the way to go?

dawehner’s picture

Status: Needs work » Needs review
FileSize
14.31 KB
9.95 KB

It is good to know that we agree on that.

This change updates the add text (thanks for the much better text!), adds the select all checkbox of core
and moves the update button.

Status: Needs review » Needs work

The last submitted patch, drupal-1828410-11.patch, failed testing.

damiankloip’s picture

FileSize
14.41 KB

I have fixed half of the tests but I think since #1798574: Refactor Views UI to be a form controller the bulk form isn't submitting properly and therefore not performing the actions.

yoroy’s picture

Status: Needs work » Needs review

Bot

damiankloip’s picture

See the comment in #13, I intentionally didn't trigger the bot.

Status: Needs review » Needs work

The last submitted patch, 1828410-13.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
14.52 KB
14.52 KB

Here is a (hopefully) fixed patch, containing the fix from #1788084: Convert actions variable(s) to CMI - add upgrade path and the do-not-test patch without that fix that we can test after that gets in.

damiankloip’s picture

FileSize
14.26 KB

Sorry, this is the correct patch to to test when we have the action module fix. Forgot to remove the action.settings.yml file.

damiankloip’s picture

FileSize
14.26 KB

Above patch is now ready for testing.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, 1828410-19.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: +VDC

#19: 1828410-19.patch queued for re-testing.

tim.plunkett’s picture

+++ b/core/modules/action/action.views.incundefined
@@ -0,0 +1,44 @@
+ * @todo Hook_views_data is used instead of hook_views_data_alter, because the
+ * alter hook doesn't load the .views.inc automatically.

The function names should have () after them, and the second line should be indented.

+++ b/core/modules/action/lib/Drupal/action/Plugin/views/field/BulkForm.phpundefined
@@ -0,0 +1,110 @@
+  function pre_render(&$values) {

Should be public I guess?

dawehner’s picture

FileSize
14.28 KB

They totally makes sense. Rerolled the patch.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, drupal-1828410-23.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +VDC

#23: drupal-1828410-23.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.31 KB
14.22 KB

Did a live patch review, and fixed the minor nitpicks.
I rerolled it myself to save @damiankloip the extra work.

sun’s picture

+++ b/core/modules/action/action.views.inc
@@ -0,0 +1,44 @@
+ * @ingroup views_module_handlers

Minor:

A @file in a group can only lead to nonsensical API output.

@ingroup and @addtogroup are supposed to be used on resources; i.e., functions, classes, interfaces.

We can fix this later after code freeze though.

+++ b/core/modules/action/action.views.inc
@@ -0,0 +1,44 @@
+/**
+ * Implements hook_views_data().
+ *
+ * @todo hook_views_data() is used instead of hook_views_data_alter(), because
+ *   the alter hook doesn't load the *.views.inc automatically.
+ */
+function action_views_data() {

I actually do not see the problem of the @todo here.

This hook implementation only adds to views data and does not alter it. Alter hooks - as the name implies - are supposed to alter data that has been collected/built in a previous step. Thus, using the alter hook to add/register information is logically and architecturally wrong.

+++ b/core/modules/action/lib/Drupal/action/Plugin/views/field/BulkForm.php
@@ -0,0 +1,110 @@
+  public function render($values) {
+    return '<!--form-item-' . $this->options['id'] . '--' . $this->view->row_index . '-->';
+  }

Is this a magic string token or something? Or is it actually "just a HTML comment"? Is this documented somewhere? Would it make sense to add a comment here to explain why this it returned as the rendered output?

+++ b/core/modules/action/lib/Drupal/action/Plugin/views/field/BulkForm.php
@@ -0,0 +1,110 @@
+      // Add the tableselect javascript and required css classes.
+      drupal_add_library('system', 'drupal.tableselect');
...
+  public function views_form(&$form, &$form_state) {

Since we're outputting a form, wouldn't it make sense to use #attached in the form? (potentially getting rid of the entire pre_render() method?)

+++ b/core/modules/action/lib/Drupal/action/Plugin/views/field/BulkForm.php
@@ -0,0 +1,110 @@
+      $this->options['element_label_class'] .= 'select-all';
...
+        '#attributes' => array('class' => array('bulk-form-select')),

Are these the regular CSS classes we're applying to tableselect/select-all/bulk-operation tables?

+++ b/core/modules/action/lib/Drupal/action/Plugin/views/field/BulkForm.php
@@ -0,0 +1,110 @@
+    $form['action'] = array(
+      '#type' => 'select',
+      '#title' => t('Action'),
...
+      '#description' => t('Select the action you want to execute on the content entitites.'),
+    );
...
+      // Replace the text with Update.
+    $form['actions']['submit']['#value'] = t('Update');

We probably want to re-evaluate the usability aspects of these strings, but I think it makes sense to do that in a follow-up patch/issue.

+++ b/core/modules/action/lib/Drupal/action/Plugin/views/field/BulkForm.php
@@ -0,0 +1,110 @@
+    // Move the submit button beside the selection.
+    $form['actions']['#weight'] = 1;

I think we actually want to duplicate the entire 'actions' container to the top of the form.

However, we can happily investigate that post-commit.

+++ b/core/modules/views/views.module
@@ -1627,6 +1633,15 @@ function views_form_views_form($form, &$form_state, ViewExecutable $view, $outpu
+  $form['actions'] = array(
+    '#type' => 'actions',
+    '#weight' => 100,
+  );

#weight 100 is the default for #type actions, it isn't really necessary to specify it here.

dawehner’s picture

FileSize
14.18 KB
10.5 KB

Thank you for the review.

Minor:

A @file in a group can only lead to nonsensical API output.

@ingroup and @addtogroup are supposed to be used on resources; i.e., functions, classes, interfaces.

Well i'm happy to discuss that in a follow issue, but this is just to make it consistent with the other files.

I actually do not see the problem of the @todo here.

This hook implementation only adds to views data and does not alter it. Alter hooks - as the name implies - are supposed to alter data that has been collected/built in a previous step. Thus, using the alter hook to add/register information is logically and architecturally wrong.

The reason why this is done is the edge-case problem of module_invoke_all and merging subarrays. It is still using array_merge_recursive,
but once NestArray:mergeDeep is used for that, we should be save to use that. It would also make things easier to understand.

Is this a magic string token or something? Or is it actually "just a HTML comment"? Is this documented somewhere? Would it make sense to add a comment here to explain why this it returned as the rendered output?

Yeah this is sadly the way currently views forms work. We render the checkboxes and then replace it on the actual output of the view.
This is all magic we should improve later, but this patch really just implements this API.

Since we're outputting a form, wouldn't it make sense to use #attached in the form? (potentially getting rid of the entire pre_render() method?)

Put the #attached into the views_form method. To be able to render the class early enough we need the pre_render function.

So how this works: The view is rendered, with the placeholders. When the actual form is rendered, the placeholder values are generated and replaced. So as the view is rendered before the css class has to be applied before.

Are these the regular CSS classes we're applying to tableselect/select-all/bulk-operation tables?

There is currently not really a standard for that, let's remove it for now.

I think we actually want to duplicate the entire 'actions' container to the top of the form.

Let's attach a screenshot how it looks at the moment :) It is already at the top.

#weight 100 is the default for #type actions, it isn't really necessary to specify it here.

form_process_actions doesn't have that behavior, at least not in the code :)

dawehner’s picture

FileSize
1.49 KB

Let's provide the interdiff as well.

sun’s picture

It is already at the top.

Yeah, I said "duplicate to the top" though. Anyway, we can do that later.

form_process_actions doesn't have that behavior, at least not in the code

Element property defaults are not defined in process callbacks, but in hook_element_info(). Check: system_element_info() ;)

dawehner’s picture

FileSize
10.48 KB
14.31 KB

I'm sorry you are so right regarding the points. To be honest, it doesn't look that good, but sure we can iterate on that later.

sun’s picture

Thank you! Let's get this sucker in :)

webchick’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
3.58 KB

I am SO EXCITED to see this!! :D

However, I did some testing and something is not quite right. I created a simple view of node titles with the bulk actions field, checked the first element and chose "Make sticky" but it made *all* of the nodes in the view sticky.

Here's the YML file for my view.

damiankloip’s picture

FileSize
14.9 KB

I have added a test to show this problem. Sorry, no fix yet though...

stevector’s picture

Status: Needs work » Needs review
FileSize
2.77 KB
15.04 KB

views_form_submit() was just missing a check for whether a given row had actually been selected.

I also had to comment out the "#return_value" and do a reset on node_load() to get the tests passing. The return_value question has a todo that should be resolved before committing.

damiankloip’s picture

FileSize
15.21 KB

@stevector, Nice work. That all looks good to me.

I have removed #return_value, as I'm pretty sure it just works for strings, not integers, so having the entity ID there doesn't help (hence it being defaulted to 'on').

I also added a count for the number of actions performed and included a drupal_set_message to display the action performed and the count.

andypost’s picture

I'm pretty sure it just works for strings, not integers, so having the entity ID there doesn't help (hence it being defaulted to 'on').

Please take care about string and integer Ids, mostly all of config entities have machine-name as ID also it's a good idea to have uuid as argument too

EDIT Related issue #1813832: Entity wrongly checks existence of ID in isNew() method

damiankloip’s picture

#37 Huh? I don't think that affects anything here.

damiankloip’s picture

FileSize
3.15 KB
15.5 KB

I changed things round a bit, so we aren't always iterating through all the view results but filtering which checkboxes are actually checked instead. Also added a couple of assertions for the drupal_set_message.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, 1828410-39.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: +VDC

#39: 1828410-39.patch queued for re-testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This seems to be good to go.

damiankloip’s picture

FileSize
15.58 KB

Rerolled.

bojanz’s picture

+1 for RTBC.

I think this is a fine first step, and gets us through the door.
Further UI improvements, batching, improved action api can and should happen in new issues.

My only comment was that I still would have liked to be able to select which actions should be available to the user on the view, but it's not something that should hold up a patch as important as this one 10 days before the feature freeze.

yoroy’s picture

Great to see this move forward so quickly. +1 for rtbc as well, but can we have a last screenshot of the full page so that we can see what we're committing here? Thank you.

dawehner’s picture

FileSize
21.05 KB

Here is a recent screenshot.

yoroy’s picture

Do I see a bit of a second 'Update' button at the bottom there? :-)

sun’s picture

I agree we should move forward here.

We should create a follow-up issue for configuring the available bulk actions. Experience with admin_views leads me to believe that this should have two essential configuration modes: A) Inclusive selection, or B) Exclusive selection.

That is, because the typical use-case of administration views is "I don't care, give me each and every available action from all modules that you have and the current user is allowed to perform, EXCEPT these." (whereas "these" would be e.g., ban_ip_action() [whereas we just removed that particular example, but there are certainly more, such as "Show a message"... ;)])

The other, existing follow-up is #1823608: Admin views in core, and I think we'll have to focus on that a bit more intensively in order to get something ready before feature freeze.

dawehner’s picture

Yes you do, see http://drupal.org/node/1828410#comment-6677988 :) Missed it on me screenshot, but you can believe me, it's there if you try to measure it.

sun’s picture

Oh, and I also think we want to create a separate follow-up issue for tweaking the UI/UX — in #10, @yoroy actually said that the button label should be "Apply" (not "Update") and I agree with that. However, I think this and potentially other tweaks are sufficiently minor to be handled in a follow-up issue, in which we can focus on the UI/UX only.

damiankloip’s picture

Yep, 100% agree with the above few comments. I would like to see this in, so we have our foundations. Then we can easily tackle other tweaks/functionality in follow ups.

sun’s picture

xjm’s picture

Thanks @sun.

webchick’s picture

Priority: Normal » Major
Status: Reviewed & tested by the community » Fixed

We are currently over thresholds, but we were under thresholds this weekend while I was at an event an unable to commit patches. So... cashing a raincheck! ;)

Committed and pushed to 8.x. YEAH!!!!!!!!

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