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/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
Line 204: Unused local variable $id
Line 353: Unused local variable $argument_id
Line 1011: Unused local variable $arg
Line 1109: Unused local variable $style
Line 1574: Unused local variable $type
Line 1689: Unused local variable $arg
Line 2080: Unused local variable $output
Comment | File | Size | Author |
---|---|---|---|
#15 | core-remove-unused-vars-displaypluginbase.php-2072593-15.patch | 3.9 KB | deneo |
#9 | core-remove-unused-vars-displaypluginbase.php-2072593-9.patch | 3.9 KB | Anonymous (not verified) |
#4 | core-remove-unused-vars-displaypluginbase.php-2072593-4.patch | 4.13 KB | Anonymous (not verified) |
#4 | 2072593-3-4-interdiff.txt | 835 bytes | Anonymous (not verified) |
#3 | core-remove-unused-vars-displaypluginbase.php-2072593-3.patch | 4.1 KB | legolasbo |
Comments
Comment #1
legolasboRemoved unused variables
Comment #2
phiit CreditAttribution: phiit commentedReviewed, patch clears all mentioned unused variables, but after removing the $arg at line 1023 (see below) $handler becomes unused variable.
Remaining code that has unused $handler starts at line 1026 with the patch applied:
Comment #3
legolasboI've had a look at the code chunk related to $handler and found a way to remove both $handler and $token. While figuring out if this code chunk is actually used i deleted it and ran almost all views tests. The tests didn't seem to break so i think this code needs tests. However i think we should check for and add missing tests for the entire class in a follow up issue.
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedI have reviewed this patch, all looks OK except you are now using a function within a loop declaration, which may cause performance issues as it may perform on each loop iteration:
I've attached a patch and interdiff to take the calculation out of the loop declaration. Tried to choose an appropriate variable name but feel free to alter:
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commented#4: core-remove-unused-vars-displaypluginbase.php-2072593-4.patch queued for re-testing.
Comment #7
boran CreditAttribution: boran commentedReviewed the patch + interdiff in #4
- all the changes make sense and are easy to read
- the patch applies cleanly
- views continues working
Comment #8
alexpottPatch no longer applies.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedPatch re-rolled. Following section removed as no longer applicable:
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedD'oh, setting to "needs review".
Comment #11
mcrittenden CreditAttribution: mcrittenden commentedTags
Comment #12
johnmcc CreditAttribution: johnmcc commentedLooks good and applies cleanly. +1.
Comment #13
areke CreditAttribution: areke commentedThis still applies and works well.
Comment #14
catch$count = 1 - missing spaces.
Comment #15
deneo CreditAttribution: deneo commentedfixed
Comment #16
catchComment #17
webchickCommitted and pushed to 8.x. Thanks!
Comment #19
oadaeh CreditAttribution: oadaeh commentedLooking only at the last patch, it looks like no tests were included. Is that true? It's also been a while, so it's possible they were added after this patch went in.