The code in actions.inc is in the process of being converted to a new actions module in #1008166: Actions should be a module. When that is complete, the variable 'actions_max_stack' (and possibly others) in that new module will need to be converted to use the new CMI sub-system.

Files: 
CommentFileSizeAuthor
#25 drupal-1788084-25.patch1.79 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 48,904 pass(es).
[ View ]
#17 1788084-convert_actions_variables_to_CMI_add_upgrade.patch512 bytesandyceo
FAILED: [[SimpleTest]]: [MySQL] 47,748 pass(es), 15 fail(s), and 15 exception(s).
[ View ]
#12 drupal-1788084-12.patch257 bytesdawehner
PASSED: [[SimpleTest]]: [MySQL] 46,394 pass(es).
[ View ]
#8 1788084-convert_actions_variables_to_CMI-8.patch2.64 KBandyceo
PASSED: [[SimpleTest]]: [MySQL] 42,306 pass(es).
[ View ]
#5 1788084-convert_actions_variables_to_CMI-2.patch2.15 KBandyceo
PASSED: [[SimpleTest]]: [MySQL] 41,729 pass(es).
[ View ]
#3 1788084-convert_actions_variables_to_CMI.patch2.15 KBandyceo
PASSED: [[SimpleTest]]: [MySQL] 41,732 pass(es).
[ View ]

Comments

In the issue summary, the possibility of other action information about being converted to CMI sub-system was raised. I am wondering whether configuration information for configurable actions should be converted directly to config files or if that info should be part of a config object associated with a action configurable plugin. Thoughts?

Component:configuration system» action.module

Status:Active» Needs review
StatusFileSize
new2.15 KB
PASSED: [[SimpleTest]]: [MySQL] 41,732 pass(es).
[ View ]

My first work with new CMI system.

Yay, thanks! :)

+++ b/core/modules/action/config/action.max_stack.yml
@@ -0,0 +1 @@
+max_stack: '35'

1) The config file/object should be named 'action.settings'.

2) 'max_stack' doesn't sound very self-descriptive to me. It's about stack overflow, and infinite recursion, ... I'm fairly sure that there is a better and more common name for such a parameter in the tech industry.

StatusFileSize
new2.15 KB
PASSED: [[SimpleTest]]: [MySQL] 41,729 pass(es).
[ View ]

1) The config file/object should be named 'action.settings'.

fixed.

2) 'max_stack' doesn't sound very self-descriptive....

As far as I know, the similar thing is the recursion limit in python language:

import sys
print(sys.getrecursionlimit())
Output:
1000

The function in python can not call itself more than 1000 times.

So I replaced "max_stack" with "stack_limit".

As far as I know, the similar thing is the recursion limit in python language

Why don't we simply name it recursion_limit then? :) That sounds most self-descriptive, at least to me.

Status:Needs review» Needs work

+1 to recursion_limit - self descriptive

+++ b/core/modules/action/action.moduleundefined
@@ -141,7 +141,8 @@ function actions_do($action_ids, $object = NULL, $context = NULL, $a1 = NULL, $a
+  $config = config('action.settings');
+  if ($stack > $config->get('stack_limit')) {
     watchdog('action', 'Stack overflow: too many calls to actions_do(). Aborting to prevent infinite recursion.', array(), WATCHDOG_ERROR);
+++ b/core/modules/action/lib/Drupal/action/Tests/LoopTest.phpundefined
@@ -61,7 +63,8 @@ class LoopTest extends WebTestBase {
+    $config = config('action.settings');
+    for ($i = 1; $i <= $config->get('stack_limit'); $i++) {
       $expected[] = "Test log #$i";
     }
     $expected[] = 'Stack overflow: too many calls to actions_do(). Aborting to prevent infinite recursion.';

Use a variable here like

<?php
$recursion_limit
= config('action.settings')->get('recursion_limit');
?>

Also I suggest to change a watchdog() message to contain a variable to describe a limit.
A kind of "Stack overflow: recursive calls to actions_do() is limited by %limit... ", array('%limit' => $recursion_limit

+++ b/core/modules/action/lib/Drupal/action/Tests/LoopTest.phpundefined
@@ -48,7 +48,9 @@ class LoopTest extends WebTestBase {
+    $config = config('action.settings');
+    $config->set('action.stack_limit', 7);
+    $config->save();

Use chain call without variable:
<?php
config
('action.settings')
  ->
set('action.stack_limit', 7)
  ->
save();
?>

Status:Needs work» Needs review
StatusFileSize
new2.64 KB
PASSED: [[SimpleTest]]: [MySQL] 42,306 pass(es).
[ View ]

Thank you all for your help!

Here is reworked patch.

Status:Needs review» Reviewed & tested by the community

Thanks! :)

Status:Reviewed & tested by the community» Fixed

Thanks a lot!

Committed and pushed to 8.x.

Status:Fixed» Closed (fixed)

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

Status:Closed (fixed)» Needs review
StatusFileSize
new257 bytes
PASSED: [[SimpleTest]]: [MySQL] 46,394 pass(es).
[ View ]

The yml file seems to be missing from the directory.

Status:Needs review» Reviewed & tested by the community

Yep, we need this in. I have added it to the patch in #1828410: Provide a bulk_form element for actions and fixes the issue.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Title:Convert actions variable(s) to CMIConvert actions variable(s) to CMI - add upgrade path
Status:Fixed» Needs work

Did we forget the upgrade path here?

Status:Needs work» Needs review
StatusFileSize
new512 bytes
FAILED: [[SimpleTest]]: [MySQL] 47,748 pass(es), 15 fail(s), and 15 exception(s).
[ View ]

Here is patch with update path.

Status:Needs review» Needs work

The last submitted patch, 1788084-convert_actions_variables_to_CMI_add_upgrade.patch, failed testing.

I guess we should also add an upgrade path test for that.

Status:Needs work» Needs review

@dawehner I see no reason to add upgrade test - this issue about 1 variable not a table

Status:Needs review» Needs work

The last submitted patch, 1788084-convert_actions_variables_to_CMI_add_upgrade.patch, failed testing.

Category:task» bug
Priority:Normal» Critical
Issue tags:+D8 upgrade path

Assigned:Unassigned» dawehner
Issue tags:-D8 upgrade path

Working on that

Assigned:dawehner» Unassigned
Status:Needs work» Needs review
StatusFileSize
new1.79 KB
PASSED: [[SimpleTest]]: [MySQL] 48,904 pass(es).
[ View ]

In contrast to the patch in #17 i moved this into system.install because previously the functionality of actions.module happened in includes/actions.inc

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

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