Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#46 | actions.png | 21.05 KB | dawehner |
#43 | 1828410-43.patch | 15.58 KB | damiankloip |
#39 | 1828410-39.patch | 15.5 KB | damiankloip |
#39 | interdiff.txt | 3.15 KB | damiankloip |
#36 | 1828410-36.patch | 15.21 KB | damiankloip |
Comments
Comment #1
dawehnerThis is the absolute simplest version you can do.
Comment #2
dawehnerIt's working fine so far, and is relative simple
Comment #4
dawehnerLet's fix the test.
Comment #6
yoroy CreditAttribution: yoroy commentedIs there UI in this patch or just plumbing? Thanks.
Comment #7
dawehner#4: drupal-1828410-4.patch queued for re-testing.
Comment #8
dawehnerI'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.
On the output it looks like that. One particular helpful feature would be to provide a table select all checkbox.
Comment #9
sunJust to provide a comparison — here's what VBO looks like in D7:
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.
Comment #10
yoroy CreditAttribution: yoroy commentedNice! 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?
Comment #11
dawehnerIt 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.
Comment #13
damiankloip CreditAttribution: damiankloip commentedI 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.
Comment #14
yoroy CreditAttribution: yoroy commentedBot
Comment #15
damiankloip CreditAttribution: damiankloip commentedSee the comment in #13, I intentionally didn't trigger the bot.
Comment #17
damiankloip CreditAttribution: damiankloip commentedHere 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.
Comment #18
damiankloip CreditAttribution: damiankloip commentedSorry, this is the correct patch to to test when we have the action module fix. Forgot to remove the action.settings.yml file.
Comment #19
damiankloip CreditAttribution: damiankloip commentedAbove patch is now ready for testing.
Comment #21
damiankloip CreditAttribution: damiankloip commented#19: 1828410-19.patch queued for re-testing.
Comment #22
tim.plunkettThe function names should have () after them, and the second line should be indented.
Should be public I guess?
Comment #23
dawehnerThey totally makes sense. Rerolled the patch.
Comment #25
tim.plunkett#23: drupal-1828410-23.patch queued for re-testing.
Comment #26
tim.plunkettDid a live patch review, and fixed the minor nitpicks.
I rerolled it myself to save @damiankloip the extra work.
Comment #27
sunMinor:
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.
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.
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?
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?)
Are these the regular CSS classes we're applying to tableselect/select-all/bulk-operation tables?
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.
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.
#weight 100 is the default for #type actions, it isn't really necessary to specify it here.
Comment #28
dawehnerThank you for the review.
Well i'm happy to discuss that in a follow issue, but this is just to make it consistent with the other files.
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.
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.
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.
There is currently not really a standard for that, let's remove it for now.
Let's attach a screenshot how it looks at the moment :) It is already at the top.
form_process_actions doesn't have that behavior, at least not in the code :)
Comment #29
dawehnerLet's provide the interdiff as well.
Comment #30
sunYeah, I said "duplicate to the top" though. Anyway, we can do that later.
Element property defaults are not defined in process callbacks, but in
hook_element_info()
. Check: system_element_info() ;)Comment #31
dawehnerI'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.
Comment #32
sunThank you! Let's get this sucker in :)
Comment #33
webchickI 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.
Comment #34
damiankloip CreditAttribution: damiankloip commentedI have added a test to show this problem. Sorry, no fix yet though...
Comment #35
stevectorviews_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.
Comment #36
damiankloip CreditAttribution: damiankloip commented@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.
Comment #37
andypostPlease 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
Comment #38
damiankloip CreditAttribution: damiankloip commented#37 Huh? I don't think that affects anything here.
Comment #39
damiankloip CreditAttribution: damiankloip commentedI 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.
Comment #41
damiankloip CreditAttribution: damiankloip commented#39: 1828410-39.patch queued for re-testing.
Comment #42
dawehnerThis seems to be good to go.
Comment #43
damiankloip CreditAttribution: damiankloip commentedRerolled.
Comment #44
bojanz CreditAttribution: bojanz commented+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.
Comment #45
yoroy CreditAttribution: yoroy commentedGreat 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.
Comment #46
dawehnerHere is a recent screenshot.
Comment #47
yoroy CreditAttribution: yoroy commentedDo I see a bit of a second 'Update' button at the bottom there? :-)
Comment #48
sunI 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.
Comment #49
dawehnerYes 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.
Comment #50
sunOh, 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.
Comment #51
damiankloip CreditAttribution: damiankloip commentedYep, 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.
Comment #52
sunKnown follow-up issues should ideally be created ahead of time, so I've done that for this case:
#1845840: Tweak the UI/UX and accessibility of the views bulk operations interface
#1845836: Make the actions exposed in the views bulk operation handler configurable
Comment #53
xjmThanks @sun.
Comment #54
webchickWe 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!!!!!!!!