BackendChain::removeBin() is broken

1. +++ b/core/lib/Drupal/Core/Cache/BackendChain.php
   @@ -224,11 +224,11 @@ public function isEmpty() {
   -   * {@inheritdoc}
   +   * Implements Drupal\Core\Cache\CacheBackendInterface::removeBin().
   

   I think this was an unintentional change.

   Should keep inheritdoc, right?

2. +++ b/core/lib/Drupal/Core/Cache/BackendChain.php
   @@ -224,11 +224,11 @@ public function isEmpty() {
      public function removeBin() {
        foreach ($this->backends as $backend) {
   -      $this->removeBin();
   +      $backend->removeBin();
        }
   

   fixes unused var $backend by... using it. :)

   Seems ok to me.

I think this needs a review? Changing status.

I reviewed the patches and Patch #3 is the correct one by using the variable instead of removing it since the other methods are doing the equivilant.

RTBC

Patch no longer applies.

why so many patches? here's another :)
also, shouldn't this a bug report, with tests, etc?

Thanks for rerolling this; the patch applies cleanly now.

herom: Actually, yes. Well done. This is a legit bug, and as a result needs a test.

Tests you can find at core/modules/system/lib/Drupal/system/Tests/Cache/BackendChainUnitTest.php (there's not a lot there atm)

Please also remove any other unused local variables for this class in this patch.

Actually, since this one has a legit bug associated with it, scratch that. Retitling to reflect the bug. :)

Ok so here is an outline of the test.

in setup() three backends are primed with same value. 't123' and then chained together

1) test that 't123' can be read.
2) backends are removed.
3) without backends assert 't123' should not be read.

test failed...

unfortunately, this can't be tested by chaining MemoryBackends (e.g. #15), because MemoryBackend's implementation of RemoveBin is empty. and I can't think of a way to verify RemoveBin using DatabaseBackend without throwing exceptions around. so, I decided to use a Mock.

The test is now in Drupal\Tests\Core\Cache\BackendChainImplementationUnitTest. I guess it's the right place, but not sure?

Also, the test-only patch would cause a fatal instead of a simple fail, because the old code keeps calling itself, so... infinite recursion! I hope this doesn't mess up the testbot.

Test failed as expected with out of memory fatal ( I don't want to know how many times that looped), fixed with the fix, back to RTBC.

Committed 1a24063 and pushed to 8.x. Thanks!

15: drupal-core-remove-unused-local-variable-2079857-11.patch queued for re-testing.

ignore this retest. testbot seemed to stop working. just testing.