Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mrsinguyen’s picture

Status: Active » Needs review
FileSize
544 bytes

Removed the unused local variable

YesCT’s picture

Status: Needs review » Needs work
  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.

mrsinguyen’s picture

Status: Needs work » Needs review
FileSize
437 bytes

Thanks for your review. Added another patch

sandipmkhairnar’s picture

Status: Needs review » Needs work
FileSize
437 bytes
simanjan’s picture

Cyberschorsch’s picture

Status: Needs work » Needs review

I think this needs a review? Changing status.

Status: Needs review » Needs work

The last submitted patch, drupal-core-remove-unused-local-variable-2079857.patch, failed testing.

Cyberschorsch’s picture

Status: Needs work » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

herom’s picture

Component: user interface text » cache system
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
437 bytes

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

areke’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Thanks for rerolling this; the patch applies cleanly now.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

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)

xjm’s picture

Title: Remove Unused local variable $backend from /core/lib/Drupal/Core/Cache/BackendChain.php » Remove unused local variables /core/lib/Drupal/Core
Priority: Normal » Minor

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

xjm’s picture

Title: Remove unused local variables /core/lib/Drupal/Core » BackendChain::removeBin() is broken
Priority: Minor » Normal

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

martin107’s picture

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.

martin107’s picture

Status: Needs work » Needs review

The last submitted patch, 15: drupal-core-remove-unused-local-variable-2079857-11.patch, failed testing.

The last submitted patch, 15: drupal-core-remove-unused-local-variable-2079857-11.patch, failed testing.

herom’s picture

Status: Needs review » Needs work

test failed...

herom’s picture

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.

The last submitted patch, 20: backend-chain-removebin-2079857-test-only.patch, failed testing.

The last submitted patch, 20: backend-chain-removebin-2079857-test-only.patch, failed testing.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

herom’s picture

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

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

The last submitted patch, 15: drupal-core-remove-unused-local-variable-2079857-11.patch, failed testing.

Status: Fixed » Closed (fixed)

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