Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Metadata asserstions for list types like list do not work.
Comment | File | Size | Author |
---|---|---|---|
#14 | interdiff.patch | 1.27 KB | geek-merlin |
#14 | rules-2103079-14b.patch | 1.23 KB | geek-merlin |
#14 | rules-2103079-14a.patch | 1.49 KB | geek-merlin |
Comments
Comment #1
fagoYes, 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.Comment #3
geek-merlinOK, 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
Comment #4
geek-merlinSo 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.
Comment #5
geek-merlinOK, here's the debug food:
Backtrace of condition assertion - DOES work:
Backtrace of action assertion - does NOT work:
Comment #6
geek-merlinGotcha. 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.
Comment #7
geek-merlinComment #8
geek-merlinhow 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)
Comment #9
hkoosha CreditAttribution: hkoosha commentedComment #10
fagoThis should use the API function entity_property_list_extract_type() instead.
Comment #11
hkoosha CreditAttribution: hkoosha commentedfixed issue at #10
Comment #12
geek-merlinOK, tested that this patch applies cleanly and still fixes the issue.
Comment #13
fagoThat 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.
This should be called $entity_type - as this is what it is/must be.
Comment #14
geek-merlinFago, 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)
Comment #15
geek-merlinEDIT: sorry testbot for the interdiff...
Comment #17
geek-merlinOK,
* 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.
Comment #18
fagoI'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.
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!