Actions reordering on views bulk forms

I produced a few alternatives and put them in the issue summary. Disclaimer: The statements of usage (frequent/infrequent) in the alternatives are assumptions based on my contact with side editors of sites build by both myself and by third parties. Feel free to replace it with facts.

Variation 1: D7-like
Pros: Familiar to D7 users
Cons: Publish is executed without confirmation

Variation 2: Safe default
Pros: 1. Safe default. Content state is not changed; 2. Freedom to choose any order for the remaining actions; 3. All frequently used operations on the top of the list.
Cons: 1. Clicking Apply will with the default result in a (friendly) (error) message; 2. New UX pattern for core

Variation 3: Safe first
Pros: Non-destructive content operation as default option
Cons: 1. Default operation is executed without warning; 2. Less frequently used operations are placed on top.

As a side note, I think the Save option should be removed. But perhaps we should create a follow-up issue for this.

Agree the current situation is not a good default. I don't think we have to come up with really new ideas here. Matching it to what we have in D7 is a good enough fix I'd say.

I investigated the code behind this drop down list.
The items in the select list are bulk operation actions and can be configured in the Content view (admin/structure/views/view/content). Similar select lists are used in the Comment admin and User admin views. The order of user actions list (admin/people) needs some love too.

The items in the list are Action plugins (e.g. Drupal\node\Plugin\Action\DeleteNode). They are sorted by their action plugin ID (e.g. node_delete_action). The action plugin does not have a weight to be sorted by.

I see a few ways to fix it:
- Add a weight to the Action plugin and and sort them by it. This includes a modification of the schema of the Action plugin.
- Let Views add a weight to the action plugins, perhaps first hard coded, later with a sort option in the Views interface.
- Reverse the current sort order. Unpublish will become the first option.

An additional solution to explore after discussion with dawehner:
- Add the property 'category' to the Action plugin and sort by category first, by ID next. Block plugin already has a 'category'.

assigning to me.

I am working on it.

Just few changes are done in order to get safer drop down list (similar as Alternative 2: Safe default).
- None - is added as first item in the list (default) and validation of option value. Reverse the current sort order can be done if it is needed.
Using weight or category gives more flexibility but I don't know what will be affected with changes should be done.

Add some tests.

This looks like:

Second solution, same as Alternative 3: Safe first.
Added weight to Action, changed schema, added default weight values and overridden function getBulkOptions.

Code is a bit simpler.
I can't solve fails in tests. When default weight values are set in core\modules\node\config\install\system.action.node_*_action.yml, get these fails. Guess that system.action.* is proper changed.
Some advice?

Trying to fix patch.

2381293-18.patch looks like:

Add image for comment #12.

Patch looks ok to me... We would need to add update hook which would load all actions, set appropriate weights on them and save them.

Thanks @slashrsm

Add hook update and tests.

+++ b/core/modules/node/node.install
@@ -218,3 +218,35 @@ function node_update_8003() {
+    // Set appropriate weights only to node actions.
+    if (array_key_exists($action_name, $node_actions_default_weight)) {

Let's set weight for the actions that are not in the list above to some sane default. 0 maybe?

0 is set as the default weight for other actions.

A little bit better documentation.

Tested issue on simplytest.me. Looks good on frontend.
Attached image

Delete is moved to bottom. Further if we don't want to change the order, it's good to move to RTBC

+1 for RTBC

Does/should the weight show up in the config entity form?

Thanks for working on this @toniteof!

@catch: I think this screenshot means the answer is no?

I don't think they have to be made reorderable(?), a fixed order keeps is better here, just like "Quit" is always the bottom one in desktop apps etc.

I also think exposing them would be a bit weird since it would be hard to change them one by one. It probably would be better to use the draggable list builder to order them all at once, but that's maybe for a follow up ?

Yhea, lets fix the bug - and then do the reodering in refinements?

Yep completely agreed we shouldn't show it unless we eventually do a draggable list, and we shouldn't do the draggable list here, just wanted to check :)

Back to RTBC. Due to the update/config schema change this should just go into 8.1.x

So this is changing an interface by adding a method which is a total BC break and we can't do even in 8.1.x. There is an issue to discuss whether entities should have interfaces see #2661926: [policy no-patch] Document that interfaces without an @api tag can have API additions during a minor release.

See for a discuss of this see the most recent comments on #306662: Add redirect option to site-wide contact forms

It doesn't change the interface, only the base class, but it probably should pending those discussions. There's also comments on #2550249: [meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation.

Tested and verified #26 patch (2381293-26.patch) on dev-8.3.x. Its working fine and "Delete Content" has moved to the bottom.
Screenshots attached.

Because of the solution chosen, seems like it'd be good for this to have Framework Manager sign-off.

However, it would be awesome to see this fixed because yuck!

+++ b/core/modules/node/node.install
@@ -218,3 +218,39 @@ function node_update_8003() {
+function node_update_8004() {

This update needs to be in system module because all actions now have weight not just the ones provided by node and the node module is not required.

I have rebase the patch from #26 against 8.4.3. I also moved the update hook from the node module to the system module.

I also rebased the patch against 8.5.x.

I have rerolled the patch in #45 against 8.6.x and fixed the syntax errors.

I have fixed the syntax errors thrown by the last patch.

I have updated the tests for 8.6.

Update the delete action plugin names.

Fix the user_add_role_action test failures and rebase against the newest version of 8.6x.

+++ b/core/modules/node/tests/src/Functional/Views/BulkFormTest.php
@@ -90,6 +90,18 @@ protected function setUp($import_test_views = TRUE) {
     $this->drupalGet('test-node-bulk-form');
     $elements = $this->xpath('//select[@id="edit-action"]//option');
     $this->assertIdentical(count($elements), 8, 'All node operations are found.');
+
+    // Check the node operations order.
+    $order = TRUE;
+    $order &= $elements[0]->getValue() == 'node_make_sticky_action';
+    $order &= $elements[1]->getValue() == 'node_make_unsticky_action';
+    $order &= $elements[2]->getValue() == 'node_promote_action';
+    $order &= $elements[3]->getValue() == 'node_unpromote_action';
+    $order &= $elements[4]->getValue() == 'node_publish_action';
+    $order &= $elements[5]->getValue() == 'node_unpublish_action';
+    $order &= $elements[6]->getValue() == 'node_delete_action';
+    $order &= $elements[7]->getValue() == 'node_save_action';
+    $this->assertTrue($order, 'All node operations are in proper order.');

I think the problem is that &= does not do logical AND. It does bitwise AND, returning an int instead of a bool.

Let's see what the testbot thinks of my version of the patch.

Here is a minimal patch that (I hope) fixes the failing tests. I did the following:

1. Restore Drupal\node\Plugin\views\field\NodeBulkForm::getBulkOptions() from the patch in #26.
2. Add weight: 0 to core/modules/media/config/optional/system.action.*.yml

I am not sure that the new test should be added to the setUp() method in the node module's BulkFormTest. This means the test is executed twice. Why not add it to testBulkForm()?

Do we want to update getBulkOptions() in NodeBulkForm or in the parent class? The update function is in the system module (as requested in Comment #41) and weights are added to all (?) the core actions, but so far we only use those weights in the node module.

Do we want to reorder the actions in the Media bulk-options list as part of this issue, or is that out of scope? (Can we make it in scope by changing the issue title?)

Oops, I misnamed my interdiff.

The previous patch actually changed the bulk actions form. It turns out that another test assumes that "Delete content" is the default action. This patch takes care of that.

1. I found no general nit picks
2. Conceptually others may be better reviewers,
3. ... simplytest.me hangs on applying it @ 8.7.x-dev.

Does it still apply on 8.7.x-dev ?

@diqidoq:

Thanks for having a look. No, the patch does not apply on 8.7.x, so it needs a re-roll. This is probably a good Novice task, so I will add the appropriate tags.

@govind.maloo:

Thanks for taking this on!

If you follow the link to the (failing) test results, and scroll to the bottom, you will find a link labeled "1 coding standards message". If you click to expand that, it points you to one line that should use the short array syntax. I think fixing that problem is a suitable Novice task.

It looks as though the failing tests were added as part of #2788777: Allow a site-specific profile to be installed from existing config, which explains why the patch in #63 did not cause them to fail. This may be a case where the tests have to be updated. In fact, I think the three tarballs under core/tests/fixtures/config_install/ need to be updated for the config changes in this issue. Feel free to work on that if you are interested, but it is also OK to fix the coding-standard issue and remove the Novice tag.

Resolved the novice issue with array syntax.

the patch can apply, so no need to reroll

+++ b/core/modules/node/src/Plugin/views/field/NodeBulkForm.php
@@ -18,4 +18,24 @@ protected function emptySelectedMessage() {
+    uasort($options, ['Drupal\Component\Utility\SortArray', 'sortByWeightElement']);

better to refactor it as uasort($options, [SortArray::class, 'sortByWeightElement']); and add to use the classname

The patch for #70 was a tricky reroll. When I applied the patch from #63 and merged with the 8.7.x branch, there was a merge conflict because ActionResourceTestBase.php in the Rest module was more-or-less moved to ActionResourceTestBase.php in the System module. I think the reroll was done properly, adding a weight key to getExpectedNormalizedEntity() in the new location.

Good work, @govind.maloo!

I have attached an interdiff comparing the patches in #70 and #73. The patch in #73 makes exactly the change it says it does. Thanks, @vacho!

I am updating the remaining tasks.

+++ b/core/modules/system/system.install
@@ -2172,3 +2172,39 @@ function system_update_8501() {
+
+    // Set appropriate weights to node actions, otherwise set 0 as default
+    // weight for other actions.
+    if (array_key_exists($action_name, $node_actions_default_weight)) {
+      $data['weight'] = $node_actions_default_weight[$action_name];
+    }

why specific to node? we have e.g. media actions too and there's more in contrib. The names are fairly unified, so instead of having the complete name for node, we could for example do a partial match for "delete" and so on.

Comment #78:

why specific to node? we have e.g. media actions too and there's more in contrib. The names are fairly unified, so instead of having the complete name for node, we could for example do a partial match for "delete" and so on.

Having the same issue with comments as well, the patch #73 works perfect for nodes, but the default action on "/admin/content/comment" is still "Delete comment".

I will try to rewrite this patch to make it work with comments as well.

That would be great @danrod. This has been a long standing usability bug, lets fix it :)

Adding a straight re-roll for 8.8.x.

However, I would like to point out also @danrod's suggestion. From what I see, if the overwrite of the method happens on the base class instead of the node override, it should work out of the box.

As for actions from other entity types, from what I can understand, in general, people in this ticket don't want delete to be first, which mostly is the case due to the letter 'D'. Other thing I noticed here is that 'Save' comes even later so we want to have the 'Delete' as the pre-last case and save as last one.

We can have the update hook set every action that contains the 'delete' string to -1, the 'save' string to 0 and all the rest to a default value -2 (since Drupal tends to count from 0 backwards when it comes to weight).
All related actions in Core share these two strings.
Then, for the other cases, e.g. comments and media, separate tickets can be opened so that the specific order is decided and checked in terms of usability.

Setting to NR so that tests run.

Adding my approach on this for review. Will fix the tests as well.

Interesting idea to move sorting, BC is a question

@andypost there were no tests up to now ensuring the order of the actions and the system update ensures that the existing actions will receive a default value.

The only case where a problem could happen is in the sorting when the entity does not have the weight set. However, SortArray has a backup for this as the sortByWeightElement calls for sortByKeyInt which starts with

$a_weight = (is_array($a) && isset($a[$key])) ? $a[$key] : 0;
$b_weight = (is_array($b) && isset($b[$key])) ? $b[$key] : 0;
if ($a_weight == $b_weight) {
  return 0;
}

So the order will not change in that case.

What other BC break might occur?

Also, attaching an attempt to fix the tests.

I am moving the update path to the post updates since we are making changes to config entities. Hook updates are meant for changes like altering databse tables.

Also, it was annoying when I was trying to apply the changes to our project. We are still running 8.7 and this is meant for 8.8 and was marked as update 8802. That could cause a issues.

I am also adding a condition where I skip cases where the weight is already set. The reason is that if someone comes across this patch before it is merged in core, they will probably find it because they already have the need for weights and possibly different weight than the one offered here.
However, the post update of the system module might run later than that user's update.
This is to ensure that the update will not overwrite any other values.

1. +++ b/core/modules/system/src/Entity/Action.php
   @@ -143,6 +151,15 @@ public function getType() {
   +   * Returns the weight of operation.
   

   This should be:

   Returns the weight of the action

   We also should move this documentation to the ActionConfigEntityInterface and replace this one with {@inheritdoc}.

   An earlier review in this issue rejected the method in the interface as a B/C break, but this was before we nailed down the B/C policy. Adding methods to interfaces is already allowed since Drupal 8.1.x (ref Backwards compatibility policy).

2. +++ b/core/modules/system/system.post_update.php
   @@ -206,3 +206,48 @@ function system_post_update_add_expand_all_items_key_in_system_menu_block(&$sand
   +  // List of node actions.
   +  $node_actions_default_weight = [
   +    'system.action.node_make_sticky_action' => -7,
   +    'system.action.node_make_unsticky_action' => -6,
   +    'system.action.node_promote_action' => -5,
   +    'system.action.node_unpromote_action' => -4,
   +    'system.action.node_publish_action' => -3,
   +    'system.action.node_unpublish_action' => -2,
   +    'system.action.node_delete_action' => -1,
   +    'system.action.node_save_action' => 0,
   +  ];
   

   Updating the actions of the node module should not happen in the system module but instead in a post update inside the Node module. This will break in sites that do not have the Node module enabled.

   We also need a similar update hook in all other modules that offer a delete action, such as the Comment module, the Media module and the User module.

3. +++ b/core/modules/system/system.post_update.php
   @@ -206,3 +206,48 @@ function system_post_update_add_expand_all_items_key_in_system_menu_block(&$sand
   +    // Set appropriate weights to node actions, otherwise set 0 as default
   +    // weight for other actions.
   +    if (array_key_exists($action_name, $node_actions_default_weight)) {
   +      $data['weight'] = $node_actions_default_weight[$action_name];
   +    }
   +    elseif (strpos($action_name, 'delete') !== FALSE) {
   +      $data['weight'] = -1;
   +    }
   +    elseif (strpos($action_name, 'save') !== FALSE) {
   +      $data['weight'] = 0;
   +    }
   +    else {
   +      $data['weight'] = -2;
   +    }
   

   This was suggested by @berdir in #2381293-78: Actions reordering on views bulk forms but I don't like this, it relies too much on magic for my taste. Checking if the machine name of an action contains the word 'delete' is not reliable.

   Even in the core actions this doesn't work, for example the action to delete a user account is system.action.user_cancel_user_action - it doesn't contain the word 'delete'.

   We should not try to babysit action defined in contrib and try to autofix them by guessing their functionality by the machine names. Let's just reliably fix the actions that are provided by core in scope of this patch.

   Contrib modules that consider it important to reorder their actions will be able to set their own weights in a post update hook once this gets in.

1. You are right, done.

2. We are not updating 'node' actions in the system hook. We are hardcoding the names of the node actions and if the name of the loaded action is included in the list, we are setting a different weight.
Since this is the first application, I do not think there is a problem performing this in the system update. However, I will move it to the node and make sure that the update does not conflict with each other and the order doesn't matter.

The only thing I don't get is what do you propose about the rest of the actions. We should update explicitly actions for all other entity types in this ticket or apply a generic default value (-2) and leave it for a follow up?

3. No strong opinion here. Reverted the changes.

Attaching an attempt for the above.

Oops, it seems that the $config_factory->loadMultiple is loading Immutable config objects so I cannot use this to load them all.
Anyway, there are only 8 of them so it is not an issue to load them one by one.

1. +++ b/core/modules/node/node.post_update.php
   @@ -34,3 +34,29 @@ function node_post_update_configure_status_field_widget() {
   +  $node_actions_default_weight = [
   +    'system.action.node_make_sticky_action' => -7,
   +    'system.action.node_make_unsticky_action' => -6,
   +    'system.action.node_promote_action' => -5,
   +    'system.action.node_unpromote_action' => -4,
   +    'system.action.node_publish_action' => -3,
   +    'system.action.node_unpublish_action' => -2,
   +    'system.action.node_delete_action' => -1,
   +    'system.action.node_save_action' => 0,
   +  ];
   

   This will change the order of the list for existing sites and I think this should never happen. New sites will get the new order but existing sites behaviour should be preserved. Then the site builders, acknowledging the new weight feature, will be able to make a decision and, if case, will reorder the actions. That being said, what we need as (post)update is to ensure a weight: 0 for existing system.action.* configs.

2. +++ b/core/modules/jsonapi/tests/src/Functional/ActionTest.php
   @@ -54,6 +54,7 @@ protected function createEntity() {
   +      'weight' => -2,
   
   @@ -97,6 +98,7 @@ protected function getExpectedDocument() {
   +          'weight' => -2,
   
   +++ b/core/modules/node/config/install/system.action.node_unpublish_action.yml
   @@ -7,4 +7,5 @@ id: node_unpublish_action
   +weight: -2
   
   +++ b/core/modules/node/node.post_update.php
   @@ -34,3 +34,29 @@ function node_post_update_configure_status_field_widget() {
   +    $data['weight'] = isset($node_actions_default_weight[$name]) ? $node_actions_default_weight[$name] : -2;
   
   +++ b/core/modules/system/system.post_update.php
   @@ -206,3 +206,27 @@ function system_post_update_add_expand_all_items_key_in_system_menu_block(&$sand
   +    // Since Drupal starts counting weights from 0 backwards, a default -2 would
   +    // allow the delete action to be set to -1 or 0 so that it never appears
   +    // first.
   +    // @see node_post_update_set_default_action_weights
   +    $data['weight'] = -2;
   
   +++ b/core/modules/system/tests/fixtures/update/system.action.goto_2815379.yml
   @@ -8,5 +8,6 @@ id: goto_2815379
   +weight: -2
   
   +++ b/core/modules/system/tests/fixtures/update/system.action.message_2815379.yml
   @@ -8,5 +8,6 @@ id: message_2815379
   +weight: -2
   
   +++ b/core/modules/system/tests/fixtures/update/system.action.send_email_2815379.yml
   @@ -8,6 +8,7 @@ id: send_email_2815379
   +weight: -2
   
   +++ b/core/modules/taxonomy/config/install/system.action.taxonomy_term_publish_action.yml
   @@ -7,4 +7,5 @@ id: taxonomy_term_publish_action
   +weight: -2
   
   +++ b/core/modules/taxonomy/config/install/system.action.taxonomy_term_unpublish_action.yml
   @@ -7,4 +7,5 @@ id: taxonomy_term_unpublish_action
   +weight: -2
   

   Why -2? The weight can be positive, zero or negative. The smaller will get on top. So, I think we need to put all existing configs on 0 except the "delete" ones which will get a positive value. This would cover also existing contrib/custom actions that will not update their config YAML files, because SortArray::sortByWeightElement() considers the lack of weight key as weight === 0. See also the previous comment. And we can move the whole (post_update logic in system module as we iterate over all system.action.* configs and those config entities are defined by system module.

   Also the update path needs tests.

+++ b/core/modules/node/node.post_update.php
@@ -34,3 +34,29 @@ function node_post_update_configure_status_field_widget() {
+    $action = $config_factory->getEditable($name);
+    $data = $action->getRawData();
+    $data['weight'] = isset($node_actions_default_weight[$name]) ? $node_actions_default_weight[$name] : -2;
+    $action->setData($data);

Also, this is doing the job but it's not quite the right way to update a config value. Using ::set() is the way:

\Drupal::getEditable($name)
  ->set('weight', NEW VALUE)
  ->save();

EDIT: but we have to use the helper for updating config entities https://www.drupal.org/node/2949630

@claudiu.cristea In general I continued the request in the ticket. The "Delete" should not be the first action. That is why I continued on the philosofy of the patches before mine. Indeed it is kinda breaking the BC and it is not like it is hard for sites to adapt to their will accordingly.
I am providing a patch that switches back to the idea of the default weight being 0. Also, updating the title of the issue so that it matches your remarks.

I am also fixing one more case of an action created with a null weight so that the missing expected weight does not make the tests fail.

Also updated the system post update function to use the new approach for updating config entities.

I am not hidding the earlier patches to keep a space for the rest to raise an argument if needed.

Oops, forgot the status change.

Ugh... Please, ignore the interdiff... I rerolled the patch to the latest develop but compared it to the previous branch (as a branch) and the result was weird.

Uploading the actual interdiff.

The interdiff is weird. Also I see that all actions have now weight 0. Shouldn't an action, such as system.action.node_delete_action have a higher weight so that it sinks to the bottom of the list? New sites will get the new order. Existing sites without config management will work as they do now. Existing sites that are managing config are owning the site config in their sync directory. Exporting the new config is part of an update, so it should be an effect of a decision.

1. +++ b/core/modules/system/system.post_update.php
   @@ -213,3 +213,17 @@ function system_post_update_clear_menu_cache() {
   +  \Drupal::classResolver(ConfigEntityUpdater::class)->update($sandbox, 'action', function ($action) {
   +    /** @var \Drupal\system\Entity\Action $action */
   

   We don't need the type hint, we can strict type in the closure signature. And I think we should use the interface.

2. +++ b/core/modules/system/system.post_update.php
   @@ -213,3 +213,17 @@ function system_post_update_clear_menu_cache() {
   +    if (empty($action->get('weight'))) {
   

   A more accurate check would be to test if is null: $action->get('weight') === NULL.

I think it makes sense to add weight to plugin annotation, this way delete action will get default "high" weight

PS: here is example #2994748: Make a way for Help Page Sections to be ordered

I think it makes sense to add weight to plugin annotation, this way delete action will get default "high" weight

@andypost, I'm not so sure. I think the logic of having the split between plugin and config is clear. The plugin provides the logic while the config is customizing a specific implementation of the plugin. Meaning we may use an action plugin in more than one place but with different customizations (different config entities). The weight, looks to me, more belonging to customization (i.e. config entity), than the plugin business logic. In a view we may want the "Delete..." action on the bottom of the list, on other circumstances we my want it on other position. It provides the same business logic but in different contexts.

The way I read the suggestion from @andypost it is that the annotation weight is the _default_ weight, to be set initially when the config is created.

This is in line with some of the other annotation values that are also being stored in the config when it is created: like the ID, label and entity type.

Having the default value in the annotation makes it possible for action plugin developers to provide a default weight without having to ship an install / update hook.

@pfrenssen @andypost, the plugin annotation providing a "default weight" sounds interesting. That meaning the config can override that value. If a config is created and no weight has been specified, then will default to the plugin default weight. This is my understanding. That being said, I think, on annotation, it should be default_weight, not simply 'weight". Then the post upgrade path will have to set 0 to keep the actual order.

Ok, then how do we proceed? do we go with @andypost's suggestion or do we wait for him to answer? Btw, I am also in favor of what @claudiu.cristea initially said. The linked issue is providing the weight property to the HelpSection plugin. Then in the HelpController the weight is being passed to the section output directly, at

      $this_output = [
        '#theme' => 'help_section',
        '#title' => $plugin->getTitle(),
        '#description' => $plugin->getDescription(),
        '#empty' => $this->t('There is currently nothing in this section.'),
        '#links' => [],
        '#weight' => $plugin_definition['weight'],
      ];

As @claudiu.cristea pointed out, the helper section plugin is a plugin that holds the information displayed in the help page and holds explicitly information on the section information.
On the contrary, an Action is a plugin holding information about, well, an action. An action is not sortable or is having any meaning for weight. The weight doesn't really make sense outside the scope of the form because it is the only place where we sort them. It is not like we are going to use the plugin manager to load all actions and we do not want the delete action to be the first one.
What we do display on the form is an Action config entity instead. This config entity is meant to be listed and shown in the UI so it makes sense for it to have a weight in case it needs custom sorting.
I scavenged a bit and found more config entities that do such a thing:

\Drupal\contact\Entity\ContactForm
\Drupal\filter\Entity\FilterFormat
\Drupal\language\Entity\ConfigurableLanguage
\Drupal\taxonomy\Entity\Vocabulary

explicitly declare the weight as a default value in the protected property as

  /**
   * The weight of the category.
   *
   * @var int
   */
  protected $weight = 0;

\Drupal\user\Entity\Role is not using the default protected $weight = 0; way but instead uses a ::preSave method to fill in the default value if left empty and then there is the case of \Drupal\search\Entity\SearchPage which is having it created in the ::postCreate method which however was based on the https://www.drupal.org/node/2004756 which was closed without action in which Berdir points out the direct assignment to the property or the ::preCreate method for setting default values (points out in general, not in comparison for the config entities).

The config entities above, if I am not mistaking, are not based on some plugin but their purpose is pretty clear.

To give you another example, let's say that in the future we create some entity that performs more than one actions in core. That entity would somehow reference more than one actions. In that case, a weight property in the plugin itself would make much more sense since it would matter in the sequence that they are executed. If we set the $weight property to the plugin now, we merely declare that Action plugin now are tied to the UI of Drupal as it carries a property that affects its appearance.

I am not going to refactor the patch any longer until we have a final decision on that. @andypost can we have your thoughts on this as well? Also @claudiu.cristea and @pfrenssen.

I have discussed this with @claudiu.cristea about if the decision is final and was concluded that we will go with the default plugin. I still have concerns on this but still, here is a candidate patch.

Also, I am restoring the previous title as according to the #101, this is still needed. Also, remarks from #101 are met.

Some fixes for the tests.

It turns out that the suggestion from @andypost has caused the bulk actions for media entities to also change order. Since this is the case, I added a test similar to the one added for the nodes.

I really like how this is looking now. Some small remarks:

--- a/core/lib/Drupal/Core/Action/ActionBase.php
+++ b/core/lib/Drupal/Core/Action/ActionBase.php
@@ -14,6 +14,13 @@
  */
 abstract class ActionBase extends PluginBase implements ActionInterface {
 
+  /**
+   * A default weight new actions.
+   *
+   * @var int (optional)
+   */
+  protected $weight = 0;

This sounds a bit strange, how about "The default weight for new actions.".

It should also be @var int to be in line with our coding standards. It is not allowed to add (optional) to a property declaration.

@@ -23,4 +30,14 @@ public function executeMultiple(array $entities) {
     }
   }
 
+  /**
+   * Returns the weight of the action.
+   *
+   * @return int
+   *   The action weight.
+   */
+  public function getWeight() {
+    return $this->weight;
+  }

Since this is public this should be documented on the Drupal\Core\Action\ActionInterface and replaced with {@inheritdoc} here.

I would also change the documentation to be clear that this returns the default weight of the action.

--- a/core/lib/Drupal/Core/Annotation/Action.php
+++ b/core/lib/Drupal/Core/Annotation/Action.php
@@ -63,4 +63,11 @@ class Action extends Plugin {
    */
   public $category;
 
+  /**
+   * The action weight
+   *
+   * @var int
+   */
+  public $weight;

This is missing a period at the end. Let's change it to The default weight the action will have in the UI. to be entirely clear what this is about.

--- a/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
+++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
@@ -89,6 +89,21 @@ protected function setUp() {
     $this->drupalPlaceBlock('local_actions_block');
   }
 
+  /**
+   * Tests that actions are sorted first by weight and then by name.
+   */
+  public function testBulkFormActionOrder() {
+    $this->drupalGet('admin/content/media');
+    $order = TRUE;
+    $elements = $this->xpath('//select[@id="edit-action"]//option');
+
+    $order = $order && $elements[0]->getValue() == 'media_save_action';
+    $order = $order && $elements[1]->getValue() == 'media_publish_action';
+    $order = $order && $elements[2]->getValue() == 'media_unpublish_action';
+    $order = $order && $elements[3]->getValue() == 'media_delete_action';
+    $this->assertTrue($order, 'All node operations are ordered alphabetically by default due to equal weight apart from the delete action which has a bigger weight.');
+  }

This test is not about node operations but media library operations.

@@ -157,4 +173,14 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
     return parent::sort($a, $b);
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function preSave(EntityStorageInterface $storage) {
+    parent::preSave($storage);
+    if ($this->weight === NULL) {
+      $this->weight = $this->getPlugin()->getWeight();
+    }
+  }

The weight should be initialized in the constructor, not in the ::preSave() method. Otherwise if we call $action->getWeight() on a newly created, unsaved Action entity we are getting NULL back which is in violation of the interface (which declares that this returns an integer).

--- a/core/modules/system/tests/src/Kernel/Action/ActionTest.php
+++ b/core/modules/system/tests/src/Kernel/Action/ActionTest.php
@@ -98,4 +98,32 @@ public function testDependencies() {
     $this->assertIdentical($expected, $action->calculateDependencies()->getDependencies());
   }
 
+  public function testDefaultWeight() {

This test is missing documentation.

--- a/core/modules/system/tests/src/Kernel/Action/ActionTest.php
+++ b/core/modules/system/tests/src/Kernel/Action/ActionTest.php
@@ -98,4 +98,32 @@ public function testDependencies() {
+    $action = Action::create([
+      'id' => 'user_add_role_action.' . RoleInterface::ANONYMOUS_ID,
+      'type' => 'user',
+      'label' => t('Add the anonymous role to the selected users'),
+      'configuration' => [
+        'rid' => RoleInterface::ANONYMOUS_ID,
+      ],
+      'plugin' => 'user_add_role_action',
+    ]);
+    $this->assertNull($action->getWeight(), 'Weight is not set during creation.');

This test is wrong, it should instead test that the correct weight (as set in the plugin annotation) is returned immediately after creation. The $action->getWeight() method should never return NULL but should always return the correct value.

I don't wanna detail it more but what about using priority instead of weight

I'm not sure that priority would be better than weight. I think weight is more in line with how we commonly name this in Drupal.

Priority is mostly used to change the order of execution, while weight is used for the order in the UI.

Agreed that we should stick to weight, which we use almost everywhere in Drupal, Symfony uses priority. The main difference is IMHO not about execution order vs order in UI, but that the values are inverted. high order => end of the list, high priority => start of the list.

@pfrenssen thanks for the review.

It should also be @var int to be in line with our coding standards. It is not allowed to add (optional) to a property declaration.

I removed that. I got it from the \Drupal\filter\Annotation\Filter's weight property actually.

The weight should be initialized in the constructor, not in the ::preSave() method.

In general, the only case where that can help is the upgrade path. Loading actions and having them with NULL weight is easier to check if they need update. However, I think you are right on wanting the weight to be set on the constructor.

I am providing an attempt to fix all remarks.

So, in terms of the __constructor / ::presave remark I had changed it but I am reverting back to the original way for the main reason that the weight is being retrieved by the plugin, but during the \Drupal\system\Entity\Action::create method does not have the plugin_id as a mandatory property.
This causes a crash when you try to create an Action object without the plugin ID passed in from the beginning.
To keep this in the constructor, I see two ways, either handle the case where the plugin is not set yet, which might cause the object to be stored without a weight, or we will include it both in the constructor and the presave which is a redundant piece of code.

The weight should be initialized in the constructor, not in the ::preSave() method. Otherwise if we call $action->getWeight() on a newly created, unsaved Action entity we are getting NULL back which is in violation of the interface (which declares that this returns an integer).

I have updated the docblock to show that it can return NULL if the weight has not been set yet.

So, in terms of the __constructor / ::presave remark I had changed it but I am reverting back to the original way for the main reason that the weight is being retrieved by the plugin, but during the \Drupal\system\Entity\Action::create method does not have the plugin_id as a mandatory property.
This causes a crash when you try to create an Action object without the plugin ID passed in from the beginning.

I had a look at the cases where this can happen and if there was maybe another way to solve it. One example is that an action entity is created in user_user_role_insert(). This is indeed a use case where the entity is created in a standard way without passing the weight. The developer expectation here is that the weight should get the default value. In this flow it is indeed a good solution to use the ::preSave() to set the value.

I have one final small suggestion:

@@ -80,6 +89,14 @@ class Action extends ConfigEntityBase implements ActionConfigEntityInterface, En
    */
   protected $pluginCollection;
 
+  /**
+   * {inheritdoc}
+   */
+  public function __construct(array $values, $entity_type) {
+    parent::__construct($values, $entity_type);
+  }
+
+

This code block is no longer needed and can be removed.

Also it looks like the upgrade path test still needs to be written.

I fixed a small underscore that I added to test the update manually, cleaned up the code and provided an update test.
Also, thanks for the guidance :D @claudiu.cristea also helped with pinpointing the update test base class :)

It looks great, now needs change record

Awesome work! Assigning for writing the change record.

Created draft change record: https://www.drupal.org/node/3083509

I reviewed the changes in #121 and they look good. RTBC from me.

+++ b/core/modules/system/src/Entity/Action.php
@@ -143,6 +152,23 @@ public function getType() {
+  /**
+   * {@inheritdoc}
+   */
+  public function preSave(EntityStorageInterface $storage) {
+    parent::preSave($storage);
+    if ($this->weight === NULL) {
+      $this->weight = $this->getPlugin()->getWeight();
+    }
+  }

+++ b/core/modules/system/tests/src/Kernel/Action/ActionTest.php
+++ b/core/modules/system/tests/src/Kernel/Action/ActionTest.php
@@ -98,4 +98,35 @@ public function testDependencies() {

I think this code is unnecessary. \Drupal\system\Entity\Action implements EntityWithPluginCollectionInterface which means all the plugin values are copied to the config entity automatically. \Drupal\Tests\system\Kernel\Action\ActionTest::testDefaultWeight() passes with this code removed.

+++ b/core/lib/Drupal/Core/Annotation/Action.php
@@ -63,4 +63,11 @@ class Action extends Plugin {
+  public $weight;

We should init this to 0. Annotations without weight will take this value. I know it's already init in ActionBase but I would do it also here.

Fixed the remarks from #126 and #127.

It seems that the tests fail.
@alexpott it was actually a false positive. The ::assertEquals does a == comparison and considers 0 equals to NULL.

There is a similar issue that explains this a bit already at https://github.com/sebastianbergmann/phpunit/issues/1717#issuecomment-11...

I tried to find a place where values were copied (or filled) over by the plugin but could not find anything.
I changed the check to ::assertSame which does not safecast anything and does a type comparison as well.
Everything seems to be returning to normal like this.
Bottom line, it seems that the test was wrong but the ::preSave was right.

So I've taken a long look at this patch and I think we need to step back a bit. At the moment weight is being set on the action plugin annotations and then also saved as a root property on the configuration entity. But when it is being saved on the config entity is being saved outside of the plugin configuration. That's not really correct it should be part of the plugin configuration. If it was, then the logic in \Drupal\Core\Config\Entity\ConfigEntityBase::preSave() would ensure that the value on the plugin and configuration are in-sync. This isn't working because weight is not being saved as part of \Drupal\system\Entity\Action::$configuration (which is where plugin config goes).

One really big question is should weight even be configurable? It doesn't have to be - it could be something defined in the annotation and leave it at that.

If we are going to make it configurable then the following things need to be addressed. However I think adding it to the annotation and therefore part of the plugin's abilities could come first and then we could make it part of configuration in a separate patch.

1. +++ b/core/modules/system/src/ActionConfigEntityInterface.php
   @@ -30,4 +30,11 @@ public function getType();
   +  /**
   +   * Returns the action weight or null if the weight has not been set yet.
   +   *
   +   * @return int|null
   +   */
   +  public function getWeight();
   
   +++ b/core/modules/system/src/Entity/Action.php
   @@ -32,6 +33,7 @@
   + *     "weight",
   
   @@ -73,6 +75,13 @@ class Action extends ConfigEntityBase implements ActionConfigEntityInterface, En
   +  /**
   +   * The action weight.
   +   *
   +   * @var int
   +   */
   +  protected $weight;
   
   @@ -143,6 +152,23 @@ public function getType() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function preSave(EntityStorageInterface $storage) {
   +    parent::preSave($storage);
   +    if ($this->weight === NULL) {
   +      $this->weight = $this->getPlugin()->getWeight();
   +    }
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getWeight() {
   +    return $this->weight;
   +  }
   

   This doesn't need to be here.

2. +++ b/core/modules/system/system.post_update.php
   @@ -213,3 +213,24 @@ function system_post_update_clear_menu_cache() {
   +  // Only update actions that have not a weight value set to avoid overriding
   +  // already fixed values.
   

   I don;t understand why we're doing this. How has a value already been fixed?

3. +++ b/core/modules/system/system.post_update.php
   @@ -213,3 +213,24 @@ function system_post_update_clear_menu_cache() {
   +  \Drupal::classResolver(ConfigEntityUpdater::class)->update($sandbox, 'action', function ($action) {
   +    $action->set('weight', 0);
   +    return TRUE;
   +  });
   

   As this is the first time actions have weight we should be getting the weight from the annotation. If we get this right re-saving the config entity will be enough to pull the default config in the plugin system.

4. +++ b/core/modules/system/tests/fixtures/update/system.action.goto_2815379.yml
   @@ -8,5 +8,6 @@ id: goto_2815379
   +weight: 0
   
   +++ b/core/modules/system/tests/fixtures/update/system.action.message_2815379.yml
   @@ -8,5 +8,6 @@ id: message_2815379
   +weight: 0
   
   +++ b/core/modules/system/tests/fixtures/update/system.action.send_email_2815379.yml
   @@ -8,6 +8,7 @@ id: send_email_2815379
   +weight: 0
   

   These configurations shouldn't need to be changed. They are part of an update test and so should be as it was at the time when the update function was written.

Just some quick inputs:

* A bit confused about the annotation vs plugin configuration explanation from @alexpott, only default config is added to the plugin configuration, and if it's not configuration then why should it be in there?

* Keep in mind that only some action plugins are configurable, most aren't, so we can't rely on adding something to the default configuration and it then just working. The UI currently also only allows to configure/add configurable action plugins, but I think we should change that.

* At the same time, if action plugins are actually configurable, then being able to configure weights as a user definitely makes sense, e.g. with #2797583: Dynamically provide action plugins for every moderation state change, you want the publish action to be shown above the archive action.