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.
Part of meta-issue #2002650: [meta, no patch] improve maintainability by removing unused local variables
File /core/lib/Drupal/Core/Cache/BackendChain.php
Line 230: Unused local variable $backend
Comment | File | Size | Author |
---|---|---|---|
#20 | backend-chain-removebin-2079857.patch | 1.36 KB | herom |
#20 | backend-chain-removebin-2079857-test-only.patch | 956 bytes | herom |
Comments
Comment #1
mrsinguyen CreditAttribution: mrsinguyen commentedRemoved the unused local variable
Comment #2
YesCT CreditAttribution: YesCT commentedI think this was an unintentional change.
Should keep inheritdoc, right?
fixes unused var $backend by... using it. :)
Seems ok to me.
Comment #3
mrsinguyen CreditAttribution: mrsinguyen commentedThanks for your review. Added another patch
Comment #4
sandipmkhairnar CreditAttribution: sandipmkhairnar commentedComment #5
simanjan CreditAttribution: simanjan commentedComment #6
CyberschorschI think this needs a review? Changing status.
Comment #8
CyberschorschI 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
Comment #9
alexpottPatch no longer applies.
Comment #10
herom CreditAttribution: herom commentedwhy so many patches? here's another :)
also, shouldn't this a bug report, with tests, etc?
Comment #11
areke CreditAttribution: areke commentedThanks for rerolling this; the patch applies cleanly now.
Comment #12
webchickherom: 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)
Comment #13
xjmPlease also remove any other unused local variables for this class in this patch.Comment #14
xjmActually, since this one has a legit bug associated with it, scratch that. Retitling to reflect the bug. :)
Comment #15
martin107 CreditAttribution: martin107 commentedOk 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.
Comment #16
martin107 CreditAttribution: martin107 commentedComment #19
herom CreditAttribution: herom commentedtest failed...
Comment #20
herom CreditAttribution: herom commentedunfortunately, 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.
Comment #23
BerdirTest 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.
Comment #24
alexpottCommitted 1a24063 and pushed to 8.x. Thanks!
Comment #25
herom CreditAttribution: herom commented15: drupal-core-remove-unused-local-variable-2079857-11.patch queued for re-testing.
ignore this retest. testbot seemed to stop working. just testing.