I created a rule with a condition nodeisoftype and action as print a systemmessage, the event was on update of content
when i tried to update a node of content type as specified in the condition, the action did not fire. i debugged and saw that i had given the content type name in capitals , 1. so i tried to edit the condition and save, upon debug still the old condition was being picked up
2. tried deleting the rule and created another, same thing happened now also, old condition was picked up
3. i tried to clear cache , the event didnt even get dispatched now.
This seems to be a major issue!
When i tried to delete the rule and created an other rule after clearing cache, i got another weird error on updating the content
TypeError: Argument 1 passed to Drupal\rules\Plugin\Condition\NodeIsOfType::doEvaluate() must implement interface Drupal\node\NodeInterface, string given in Drupal\rules\Plugin\Condition\NodeIsOfType->doEvaluate() (line 40 of modules/contrib/rules/src/Plugin/Condition/NodeIsOfType.php)
| Comment | File | Size | Author |
|---|---|---|---|
| #27 | 2769511-27.increase-allowed-exceptions-in-travis.patch | 853 bytes | jonathan1055 |
| #21 | 2769511-21.interdiff-19-21.txt | 764 bytes | jonathan1055 |
| #21 | 2769511-21.invalidate-rules-cache-tags.patch | 9.65 KB | jonathan1055 |
Comments
Comment #2
sukanya.ramakrishnan commentedTo add an update, if i create everything properly the first time around, the rule works fine, but any updates or corrections dont work at all..
i was able to work through the php error by using the data selector instead of typing the value as node for for the nodeisoftype condition.!
Comment #3
sukanya.ramakrishnan commentedUpdate: There is still a need for a cache clear to be done if a rule is changed , but event is dispatched properly upon cache clear (Issue #3 no longer exists)
Comment #4
fagoYes, I also noted this. Looks like the cache entries are not cleared when a rule is updated. We should add some test coverage for this also.
I think the case of add/removing events is handled, but it is not handled when just some other changes are made.
Comment #5
sukanya.ramakrishnan commentedThanks fago, I also noticed one more issue related to cache! when i create a component, i have to clear the cache for the component to appear on the actions dropdown. Is that working as designed?
Thanks,
Sukanya
Comment #6
fago>when i create a component, i have to clear the cache for the component to appear on the actions dropdown. Is that working as designed?
No, of course not. You should never have to manually clear the cache. Could you file another issue for that? Thanks!
Comment #7
sukanya.ramakrishnan commentedNever mind, the issue reported in #5 does not occur. must have been an oversight on my part!
Comment #8
samuel.mortensonLooking into this a little bit - since Rules cache is by event, would the appropriate fix be to invalidate all cache entries that start with the Event which triggers a given Rule?
For example, we would invalidate cache when editing a Rule triggered by the "After saving new content" event by clearing all cache IDs that match "rules_event:rules_entity_insert:node:*".
Am I understanding this correctly?
Comment #9
promo-il commentedI use in mysql
TRUNCATE `cache_rules`;
Comment #10
Syntapse commentedIm building with Drupal 8.7.8 and refreshing cache on every single small change to Rules to make it work. Is there a fix for this in the pipeline or is it by design? thanks.
Comment #11
tr commentedComment #12
jonathan1055 commentedI have done some work on this, based on samuel.mortenson's comments in #8
This patch demonstrates the problem, by removing the
drupal_flush_all_caches();in the configureAndExecute test. This should fail.Comment #14
jonathan1055 commentedHere is the same change to the test as per #12 with the new changes to set a tag when the cache is saved (e.g. after doing something that reacted to a rules event), and invalidate the tag when the rule is changed. The tag is simply the rules event id, for example 'rules_entity_presave:node' or 'rules_entity_update:node--article'.
This patch clashes with my other one in #3108494-16: Exception when updating cache with empty data but the actual fix is independent. So which ever of these is committed first, the other will need re-rolling.
Comment #15
bmunslow commentedHi @jonathan1055,
Thanks for your work on this issue.
It turns out #3108494-16 has already been commited to dev, so, as you mentioned, #14 doesn't apply any longer.
I re-rolled your patch for latest dev, please find it attached.
Comment #16
jonathan1055 commentedThanks for the re-roll @bmunslow. I am adding further test coverage for this fix.
If any else of the 21 followers would like to test this manually, please do :-)
Comment #17
bmunslow commentedHi @jonathan1055!
I've manually tested this patch quite thoroughly in a current project and I can confirm it has fixed all caching issue for good in my case.
Comment #18
jonathan1055 commentedHere is a patch with a new test to cover the addition of two rules both reacting on the same event. This has the initial fix as included in #14/15 but I also discovered that when deleting a rule and another rule is still active with the same event then the cache for the events should also be invalidated. This is simple to fix, but here is the new test without this, so the patch will fail.
Comment #19
jonathan1055 commentedError in patch, test aborted. Patch #19 should fail properly
Comment #21
jonathan1055 commentedAs expected, the test fails after deleting Rule 1 but leaving Rule 2 active.
Patch #21 includes the fix to
ReactionRuleStorage::deletewhich is very similar to the fix in::saveForgot to say in #18 that I have added a new function
clickLinkByHref()into RulesBrowserTestBase which finds a link matching a partial href via xpath. This makes it easier to use the enable and disable links/buttons in the test.Comment #23
tr commentedActually the patch was fine - I triggered a retest and it ran as expected. Aborts are usually due to transient testbot infrastructure fails - can't find database and things like that. A retest usually works.
I'll get committed #21 later today.
Comment #24
jonathan1055 commentedWhat I meant was that I made an error in creating that patch, and I aborted the test manually. I wondered why it was run again later.
Excellent, thatnk you.
Comment #26
tr commentedThis is a very nice solution to a long-standing problem. Thanks! Committed patch #21.
Comment #27
jonathan1055 commentedThanks, yes the solution was nice and neat.
Following the recent addition of extra tests in this issue and in
#3110461: Add rules_ban actions and conditions to functional tests
#3108494: Exception when updating cache with empty data and
#3104328: Move list builders from D8RE into Rules please could you commit this patch to increase the number of allowed deprecations in Travis builds. I thought I would wait until there were several issues that changed the totals before making the patch. Keeping these numbers accurate is a good way to for me to be alerted to any new @trigger_error deprecations, which I will then report on #3089502: [meta] Rules deprecated code
Comment #28
tr commentedSure, I can commit this, but can you put the patch in the deprecation issue where we've been handling this?