Given an import with "Add a variable" actions, the provided variables used in any action that requires a specific variable type trigger the integrity check error:

The bug seems to be in RulesAbstractPlugin::import() where setUp() is called prematurely. I can't find a reference to why it is called before importing settings. As far as I can see, importSettings() and importParameterSetting() do not use set-up info in any way, which should mean it would be safe to call afterwards.

This problem is how info_alter is performed without settings, blocking subsequent attempts to actually set up the plugin and initialize data types for integrity check.

The following is a sample export. The original configuration does not produce integrity check errors. Importing it, however, will fail.

{ "rules_test" : {
    "LABEL" : "Test",
    "PLUGIN" : "rule",
    "REQUIRES" : [ "rules" ],
    "USES VARIABLES" : { "text_list_param" : { "label" : "Text list parameter", "type" : "list\u003Ctext\u003E" } },
    "DO" : [
      { "variable_add" : {
          "USING" : { "type" : "list\u003Ctext\u003E" },
          "PROVIDE" : { "variable_added" : { "text_list" : "Text list" } }
        }
      },
      { "LOOP" : {
          "USING" : { "list" : [ "text-list-param" ] },
          "ITEM" : { "text" : "Text" },
          "DO" : [ { "list_add" : { "list" : [ "text-list" ], "item" : [ "text" ] } } ]
        }
      }
    ]
  }
}
CommentFileSizeAuthor
#1 1673118-01-Set-up-abstract-plugin-after-import-settings.patch864 byteszhangtaihao
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

zhangtaihao’s picture

zhangtaihao CreditAttribution: zhangtaihao commented
FileSize
1673118-01-Set-up-abstract-plugin-after-import-settings.patch864 bytes

If I had to guess the reason for the comments, I would say that 1b601d4 implemented rule configuration import to set up plugin with the intention to use pluginParameterInfo() for import, and then 25198f9 replaced the call with $export directly so as to not invoke any behavior requiring a completely configured element.

Is this the right approach? Does it need tests? Perhaps with the export in the OP.

zhangtaihao’s picture

zhangtaihao CreditAttribution: zhangtaihao commented
Title: Abstract plugins do not perform info_alter on imported settings » Abstract plugins do not alter info using imported settings
Status: Active » Needs review

Slightly corrected title.

mitchell’s picture

mitchell CreditAttribution: mitchell commented
Assigned: Unassigned » fago
mitchell’s picture

mitchell CreditAttribution: mitchell commented

I spoke to klausi on irc about this, and he said that he wasn't sure if this approach was right or not and that would rather wait for fago's response. I personally see adding this test case to rules_tests is important.

@zhangtaihao: How did this initially come up; could you please speculate why this hasn't arisen earlier? How does this relate to solving #1698296: [Meta] Merge Conditional Rules? Do you anticipate fago having an alternative approach?

zhangtaihao’s picture

zhangtaihao CreditAttribution: zhangtaihao commented

This wouldn't actually have any impact per se on anything else (or any more than it does Rules itself). The main reasons this hasn't really struck before are (1) the limited usage of list variables created and populated in the rule itself and (2) the fact that this only happens when importing.

Because loops aren't traditionally used with created lists (as they would be if you had "While" to dynamically populate a list), using them with existing multi-valued field data wouldn't be an issue because the entity bundle assertion would've taken care of identifying the data type (e.g. list<text> for a multi-valued text field).

Using actions like "Add a variable" has the effect of late type binding in the usage of subsequent actions (through RulesPluginImplInterface::info_alter(). Rules created in the UI are fine because the "Add a variable" action would've been created with no integrity constraints (i.e. no subsequent action using a provided variable). Once it's in, Rules can take care of assembling provided variables into the parent's state variables so that a subsequent action is aware of the variable.

When importing, the unexpected behavior in the OP means the action is set up before the settings are processed (into late bindings). Without a gap in the UI workflow to let the action reinitialize on another page load, the provided variable simply has a type of "unknown", which is the default value implemented in rules_data_action_info() for 'variable_add'. When the integrity check hits a subsequent action that relies on it, for example, being a list, it'll simply fail to import.

I've only hit this problem when using Conditional Rules in some sites I'm building. I only realized it wasn't limited to Conditional Rules when I tried the import in the OP.

zhangtaihao’s picture

zhangtaihao CreditAttribution: zhangtaihao commented

I've only begun to really dig into the guts of the import process in the last few weeks, so I'm not sure if fago had to do it in the way he did in order to solve a race condition somewhere else.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago at drunomics commented
Assigned: fago » Unassigned
Status: Needs review » Postponed (maintainer needs more info)

Is this still needed for conditional rules? It seems to do fine without?

TR’s picture

TR CreditAttribution: TR commented
Status: Postponed (maintainer needs more info) » Closed (works as designed)

Closing this for now, since no further information has been provided.

Feel free to reopen if this is still a problem. As stated in #4, we would need a test case to demonstrate the problem and to demonstrate that the fix works.