Metadata asserstions for list types like list do not work.

CommentFileSizeAuthor
#14 interdiff.patch1.27 KBgeek-merlin
#14 rules-2103079-14b.patch1.23 KBgeek-merlin
#14 rules-2103079-14a.patch1.49 KBgeek-merlin
#11 0002-Fixed-2103079-Rules-metadata-assertions-for-list-typ.patch1.62 KBhkoosha
#6 0001-Fixed-2103079-Rules-metadata-assertions-for-list-typ.patch1.63 KBgeek-merlin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Status: Active » Fixed

Yes, it should work for actions as well. I never tried it though.

However, you can provide an variable with better metadata - i.e. providing it as list<text> would work fine.

Status: Fixed » Closed (fixed)

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

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin commented
Status: Closed (fixed) » Active

OK, tried it and unfortunately it does not work.

The situation:
* We need a "polymorphic" rulea action efq_rules_action_execute() that is declared returning list<entity>, but we want to provide metadata assertions like like list<node>, so we loop over it and assign directly to a node variable
* See source: declaration, action (which needs some wrapper cleanup ;-), assertion
* Debugging shows that rules
** calls the assertion
** collects the info so RulesContainerPlugin::availableVariables['fetched_entities']['rules_assertion']['#info']['type'] == 'list<node>'
** but never calls RulesData::applyMetadataAssertions() to merge this into RulesContainerPlugin::availableVariables['fetched_entities']['type']
** so we can not assign our list item to a node variable

Solutions
* Of course we can just tell the metadata wrapper what's in it but this does not give us "compile time" variable info so variable assignments will not work
* Of course we can just provide multiple actions instead of one polymorphic one but this just clutters up the action list
* So the best is to fix rules to merge in metadata assertions but we need some guidance for this

Hope this is clear, all the best
axel

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin commented
Title: Is it possible for a rules action to provide metadata assertions? » Rules action metadata assertions broken
Category: Support request » Bug report
Issue summary: View changes

So in fact i consider it a bug: Unlike conditions, action metadata assertions are not merged into metadata, see comment above.

Direction from fago where to fix this would substantially help.

If someone else (maybe me if i find the time) wants to help, one could dump the call backtrace of a action metadata assertion (which is broken) vs. a condition metadata assertion (which works) and attach it in this issue.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin commented

OK, here's the debug food:

Backtrace of condition assertion - DOES work:

call_user_func_array('rules_condition_entity_is_of_type_assertions', Array) faces.inc:123
FacesExtendable->__call('assertions', Array) rules.core.inc:297
RulesExtendable->__call('assertions') rules.core.inc:1499
RulesAbstractPlugin->variableInfoAssertions() rules.core.inc:2009
RulesContainerPlugin->variableInfoAssertions() rules.plugins.inc:265
Rule->stateVariables(Object) rules.core.inc:675
RulesPlugin->availableVariables() ui.data.inc:49
RulesDataUI::selectionForm('value', Array, Array, Object) 
call_user_func(Array, 'value', Array, Array, Object) ui.core.inc:319
RulesPluginUI->getParameterForm('value', Array, Array, 'selector') ui.core.inc:233
RulesPluginUI->form(Array, Array, Array) ui.core.inc:924
RulesAbstractPluginUI->form(Array, Array, Array) 
call_user_func_array(Array, Array) faces.inc:130
FacesExtendable->__call('form', Array) rules.core.inc:297
RulesExtendable->__call('form', Array) rules.core.inc:1182
RulesPlugin->form(Array, Array, Array) ui.forms.inc:285
rules_ui_add_element(Array, Array, Object, 'action', Object, 'admin/config/workflow/rules/components', 'action') 
call_user_func_array('rules_ui_add_element', Array) form.inc:799
drupal_retrieve_form('rules_ui_add_element', Array) form.inc:452
drupal_rebuild_form('rules_ui_add_element', Array, Array) form.inc:931
drupal_process_form('rules_ui_add_element', Array, Array) ajax.inc:370
ajax_form_callback() 
call_user_func_array('ajax_form_callback', Array) menu.inc:517
menu_execute_active_handler() index.php:21

Backtrace of action assertion - does NOT work:

call_user_func_array('efq_rules_action_fetch_entities_ekran_assertions', Array) faces.inc:123
FacesExtendable->__call('assertions', Array) rules.core.inc:297
RulesExtendable->__call('assertions') rules.core.inc:1499
RulesAbstractPlugin->variableInfoAssertions() rules.core.inc:1998
RulesContainerPlugin->stateVariables(Object) rules.plugins.inc:263
Rule->stateVariables(Object) rules.core.inc:675
RulesPlugin->availableVariables() ui.data.inc:49
RulesDataUI::selectionForm('value', Array, Array, Object) 
call_user_func(Array, 'value', Array, Array, Object) ui.core.inc:319
RulesPluginUI->getParameterForm('value', Array, Array, 'selector') ui.core.inc:233
RulesPluginUI->form(Array, Array, Array) ui.core.inc:924
RulesAbstractPluginUI->form(Array, Array, Array) 
call_user_func_array(Array, Array) faces.inc:130
FacesExtendable->__call('form', Array) rules.core.inc:297
RulesExtendable->__call('form', Array) rules.core.inc:1182
RulesPlugin->form(Array, Array, Array) ui.forms.inc:285
rules_ui_add_element(Array, Array, Object, 'action', Object, 'admin/config/workflow/rules/components', 'action') 
call_user_func_array('rules_ui_add_element', Array) form.inc:799
drupal_retrieve_form('rules_ui_add_element', Array) form.inc:452
drupal_rebuild_form('rules_ui_add_element', Array, Array) form.inc:931
drupal_process_form('rules_ui_add_element', Array, Array) ajax.inc:370
ajax_form_callback() 
call_user_func_array('ajax_form_callback', Array) menu.inc:517
menu_execute_active_handler() index.php:21
geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin commented
Title: Rules action metadata assertions broken » Rules metadata assertions for list types broken
Priority: Normal » Major
Status: Active » Needs review
Related issues: +#2103105: Provide entity type metadata for execute action
FileSize
0001-Fixed-2103079-Rules-metadata-assertions-for-list-typ.patch1.63 KB

Gotcha. The problem was that here like in some other places the list types need some extra love.

Patch flying in. Daring to raise prio as this blocks stable release of efq rules module.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin commented
Issue summary: View changes
geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin commented

how to test:
* install efq_rules from above linked sandbox
* create a rule with a "execute efq and fetch entities" action configured to fetch (say) nodes
* verify that subsequent actions can access node title in the data explorer
(which was not possible before that patch)

hkoosha’s picture

hkoosha CreditAttribution: hkoosha commented
Status: Needs review » Reviewed & tested by the community
fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Status: Reviewed & tested by the community » Needs work 
+++ b/includes/rules.state.inc
@@ -508,11 +508,16 @@ class RulesData  {
+            $is_list_type = preg_match('/list<(.*)>/', $assertion['type'], $matches);

This should use the API function entity_property_list_extract_type() instead.

hkoosha’s picture

hkoosha CreditAttribution: hkoosha commented
Status: Needs work » Needs review
FileSize
0002-Fixed-2103079-Rules-metadata-assertions-for-list-typ.patch1.62 KB

fixed issue at #10

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin commented
Status: Needs review » Reviewed & tested by the community

OK, tested that this patch applies cleanly and still fixes the issue.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/includes/rules.state.inc
@@ -508,11 +508,16 @@ class RulesData  {
-          if (isset($assertion['type']) && $var_info[$parts[0]]['type'] == 'entity') {
-            if (entity_get_info($assertion['type'])) {
-              $var_info[$parts[0]]['type'] = $assertion['type'];
+          if (isset($assertion['type'])) {

    That part is really hard to follow now. Can we improved that a bit?

    Suggestions:
    - Keep the initial entity check in the top but check for entity + list initially. That should help giving context + makes $generic_type variable useless. 

  2. +++ b/includes/rules.state.inc
@@ -508,11 +508,16 @@ class RulesData  {
+            $basic_type = $list_type ? $list_type : $assertion['type'];

    This should be called $entity_type - as this is what it is/must be.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin commented
FileSize
rules-2103079-14a.patch1.49 KB
rules-2103079-14b.patch1.23 KB
interdiff.patch1.27 KB

Fago, you are completely right. Re-reading that code, it looks way too entangled.

Source of the entanglement was the "feature" to silently ignore "non-list-preserving" assertions like entity => list<node> or list<entity> => node.
But hey, that need not be our concern and may even be valid use cases.

So code can be simplified. See patch 14a.

Then i wondered why we even need to silently ignore assertion of nonvalid entities? If we uninstall an entity type provider the rule breaks anyway.

Dropped this too and see patch 14b.

Choose one ;-) (i prefer 14b)

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin commented
Status: Needs work » Needs review

EDIT: sorry testbot for the interdiff...

Status: Needs review » Needs work

The last submitted patch, 14: interdiff.patch, failed testing.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin commented
Status: Needs work » Reviewed & tested by the community

OK,
* code is already reviewed
* both patch versions satisfy the bot and apply cleanly

so to speed things up i dared to do the last test myself
* both patches applied on a clean system fix the original issue

feel free to set needs-review.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Title: Rules metadata assertions for list types broken » Allow providing Rules metadata assertions for lists of entities
Status: Reviewed & tested by the community » Fixed

I'm not sure about allowing "entity => list or list => node" assertings, that might result in some unexpected behaviours. Howsoever, I do not think we have to check that here but can trust the condition/action to provide a reasonable assertion as we've done before.

Then i wondered why we even need to silently ignore assertion of nonvalid entities? If we uninstall an entity type provider the rule breaks anyway.

Yes, that can break and will lead to integrity check violations anyway. So that's fine.

Thus, I went ahead and committed 14.b. Thanks!

  • Commit 4867067 on 7.x-2.x authored by axel.rutz, committed by fago: 
    Issue #2103079 by axel.rutz, lootoo: Allow providing Rules metadata...

Status: Fixed » Closed (fixed)

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