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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

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?

sun’s picture

Component: configuration system » action.module
andyceo’s picture

Status: Active » Needs review
FileSize
2.15 KB

My first work with new CMI system.

sun’s picture

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.

andyceo’s picture

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".

sun’s picture

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.

andypost’s picture

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

$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:

config('action.settings')
  ->set('action.stack_limit', 7)
  ->save();
andyceo’s picture

Status: Needs work » Needs review
FileSize
2.64 KB

Thank you all for your help!

Here is reworked patch.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! :)

webchick’s picture

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.

dawehner’s picture

Status: Closed (fixed) » Needs review
FileSize
257 bytes

The yml file seems to be missing from the directory.

sun’s picture

Status: Needs review » Reviewed & tested by the community
damiankloip’s picture

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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

andyceo’s picture

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

Did we forget the upgrade path here?

andyceo’s picture

Status: Needs work » Needs review
FileSize
512 bytes

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.

dawehner’s picture

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

andypost’s picture

Status: Needs work » Needs review
andypost’s picture

@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.

catch’s picture

Category: task » bug
Priority: Normal » Critical
Issue tags: +D8 upgrade path
dawehner’s picture

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

Working on that

dawehner’s picture

Assigned: dawehner » Unassigned
Status: Needs work » Needs review
FileSize
1.79 KB

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

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community
Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

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