Exception when updating cache with empty data

Patch attached. See #3105760-8: Empty Rules are firing for more details and a debug output file.

The exception was only produced at core 8.7 because at core 8.8 and 8.9 DatabaseBackend::doSetMultiple() now has a check to exit early if $values is empty, which is not in the 8.7 code. However, the change to Rules is good to keep for 8.8+ as it makes sense not to call the cache update anyway if there is no data.

I will use the knowledge of test failures from #3106316: Scheduler Rules integration - failures at Core 8.7 - Rules cache update problem to amend Rules tests to cover this problem.

Here is an addition to testConfigureAndExecute which triggers the same rule again. This should fail at core 8.7

Interesting that the exception message in the failed 8.7 test on d.o

1) Drupal\Tests\rules\Functional\ConfigureAndExecuteTest::testConfigureAndExecute
Exception: Warning: array_fill(): Number of elements must be positive
Drupal\Core\Database\Query\Upsert->getInsertPlaceholderFragment()() (Line: 172)

is slightly different from the one I see locally

Drupal\Core\Entity\EntityStorageException
SQLSTATE[21S01]: Insert value list does not match column list:
1136 Column count doesn't match value count at row 1:
INSERT INTO {cache_rules} (cid, expire, created, tags, checksum, data, serialized) VALUES () ON DUPLICATE KEY UPDATE cid = VALUES(cid), expire = VALUES(expire), created = VALUES(created), tags = VALUES(tags), checksum = VALUES(checksum), data = VALUES(data), serialized = VALUES(serialized); Array()
in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 847 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php

They both stem from Drupal\Core\Database\Query\Upsert but the d.o. exception is found eralier in the processing. Probably some difference in php version or other backend stuff. No matter, we are not checking the exception, our test would still fail the new assertion. Here is a patch with the combined code from #2 and #5

I can see that between #8 and #10 you have taken out the new bit of test code from #6. The failure has moved from line 115 to 107 so I think it is actually in the new test code lines. The first $assert->pageTextNotContains('Title matched "Test title"!') after disabling the rule. Does the cache need to be cleared for the disable to become effective?

Thanks for the explanation and background. It has allowed me to investigate and I have solved this. Will explain and post patch tomorrow, sorry no time now, but did not want you to waste time doing what I have just done.

Well, I may have been a bit hasty, I have partially solved this. Enabling a rule works fine, just like adding a rule. If the event is not in the $events_before array then the drupalKernel->rebuildContainer() is called as required. However, when disabling a rule, the opposite check now also needs to be done. The event will always be in $events_before list. We need to create an $events_after list and if the event is not in that array, i.e. the disabled rule removed the last use of that event, then we also do the drupalKernel->rebuildContainer().

Attached is a patch which contains this amendment, and the tests should pass. This properly fixes the optimization for registered events, but it is not the end of the problem. If two rules both react on the same event, then when the first is disabled the container is not rebuilt. The disabled rule still fires and we need to clear the cache to make it not fire. #2769511: Cache is not invalidated when Rules are changed is the likely cause/solution. It is only when the second rule is disabled and the event is no longer used at all, then we get neither rule firing.

If this patch passes, I would recommend committing it, as it does most definitely fix the original problem - that of attempting to update the cache with null parameter, which is causing real errors at 8.7. Then we can work on the optimisation/kernel rebuild/cache-clear problem separately. Extra test coverage will also be needed to test adding/disabling/enabling/deleting two rules with the same event.

Thanks for the background info, it is very helpful.

I agree this is happening, but I don't know why this is a problem.

Maybe at most we'd have to invalidate a cache tag here and there to let core know ...

I have done some work on the caching and posted a patch in #2769511-14: Cache is not invalidated when Rules are changed which fixes this. You are right that the container does not need to be rebuilt in between, but we do need to invalidate the cache tags.

Unfortunately this patch clashes with that one, so which ever is committed first, the other will need re-rolling. Hopefully we can get both of these issues committed soon.

Do we even need the RulesComponentRepository anymore?

I don't know. I have no idea and no way of telling at present. But it sounds like a piece of work which is out of scope of this issue. If we do embark on that, then it would be important to be starting from a place where relevent known bugs are fixed and we have test coverage for it.

The patch in #16 above demonstrably fixes two bugs (updating cache with null value and not rebuilding the container on change of registered events), includes test coverage for both the bugs and also turns on the enable/disable buttons functionality, so it would be very nice if you could commit this. Then I can extend my patch on #2769511: Cache is not invalidated when Rules are changed to add a new test for creating two rules reacting on the same event and edit/disable/re-enable/delete and verify that the changes are effective immediately without any manual cache-clear.

It seems to me that we could use cache tags when we do the $this->cacheBackend->setMultiple() and invalidate the tags at the appropriate times

Yes, that is exactly what I have added on patch #14 in that other issue.

I think both of these issues should be done first, then we will have fixed three bugs and added test coverage, which will allow a proper investigation into removing RulesComponentRepository if we decide to do that.

Thanks. Yes, that likewise sounds reasonable. All good here so RTBC and when committed I will finish off #2769511: Cache is not invalidated when Rules are changed