Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
core/includes/menu.inc
- Unused local variable $actions (line 1885)
- Unused local variable $current (line 2005)
Comment | File | Size | Author |
---|---|---|---|
#43 | 2002726-remove-unused-vars-menu-43.patch | 2.17 KB | longwave |
Comments
Comment #1
neochief CreditAttribution: neochief commentedComment #2
neochief CreditAttribution: neochief commentedComment #3
neochief CreditAttribution: neochief commentedOk, here's the better one.
Comment #5
neochief CreditAttribution: neochief commented#3: performance-2002726.patch queued for re-testing.
Comment #7
kerasai CreditAttribution: kerasai commented#3: performance-2002726.patch queued for re-testing.
Comment #9
kerasai CreditAttribution: kerasai commentedThis is failing on Drupal\simpletest\Tests\SimpleTestTest. Unfortunately there is no further context on what's failing.
As well, there are many "Only variables should be passed by reference" type exceptions (not failures). I believe those are being generated by the reset being used like this:
Maybe a way to get around this would be something like this. This seems to defeat the purpose of the issue, which is to improve performance.
Comment #10
YesCT CreditAttribution: YesCT commentedI'm not sure. Lets see if we can get some eyes on this.
Comment #11
somepal CreditAttribution: somepal commentedalthough array_shift() also uses reset internally, lets find out if test succeeds.
Comment #13
somepal CreditAttribution: somepal commented#11: 2002726_unused-local-var_11.patch queued for re-testing.
Comment #15
martin107 CreditAttribution: martin107 commentedPlease find attached new.patch which is a simple reroll.
Two strict warning were observed during drupal installation ONLY after the patch was applied
and a quick inspection confirms it is changes made by the patch
Comment #16
martin107 CreditAttribution: martin107 commentedComment #18
martin107 CreditAttribution: martin107 commentedthis next patch removes the strict warning messages in the images associated with comment #15
and the errors in simpleTestTest .. at least on my machine
in two places have removed array_shift lines with list like line see below
$menu = array_shift(menu_router_build());
list($menu,) = menu_router_build(TRUE);
Comment #19
YesCT CreditAttribution: YesCT commentedwould just
list($menu) = menu_router_build(TRUE);
work?
---
@chx did this cool thing and pasted it to me in irc
php -r 'list($x) = array(1,2);var_dump($x);list($x,)=array(1,2);var_dump($x);list(,$x)=array(1,2);var_dump($x);'
to show: yes, list($x) is enough
but if it is a second param, need to be like: list(, $x)
Comment #20
MichaelLund CreditAttribution: MichaelLund commentedRe-rolling patch in comment #18, since it never got committed
Comment #20.0
MichaelLund CreditAttribution: MichaelLund commentedUpdated issue summary.
Comment #21
parthipanramesh CreditAttribution: parthipanramesh commentedSorry but the latest patch needs work. I can't apply this patch without errors..
Comment #22
royal121 CreditAttribution: royal121 commented20: drupal8.other_.2002726-20.patch queued for re-testing.
Comment #24
royal121 CreditAttribution: royal121 commentedComment #25
areke CreditAttribution: areke commentedThis patch doesn't apply anymore, so it needs to be re-rolled. Also, doesn't removing $key reduce readability? To me it does, which makes me think it should be left the way it is.
Comment #26
xjmComment #27
Salah Messaoud CreditAttribution: Salah Messaoud commentedstarting work
Comment #28
Salah Messaoud CreditAttribution: Salah Messaoud commentedpatch re-rolled to the latest version
Comment #29
Salah Messaoud CreditAttribution: Salah Messaoud commentedComment #30
elgordogrande CreditAttribution: elgordogrande commentedPrior to applying this patch, I have inspected the code using PhpStorm and my understanding is that the submitted patch removes two unused variables, but the menu.inc file contains four unused variables ($action_count on line 2070, $current on line 2125, $key on line 2361, $key on line 2391). Was there some reason I'm not aware of that the first two were not removed? If not, might I suggest that you remove all four and re-roll?
Comment #31
parthipanramesh CreditAttribution: parthipanramesh commentedEDIT: Now I also think that @elgordogrande is right. Sorry
Comment #32
star-szrComment #33
areke CreditAttribution: areke commented@elgordogrande is right. These other unused variables should also be taken care of in this patch.
Comment #34
Salah Messaoud CreditAttribution: Salah Messaoud commentedComment #35
Salah Messaoud CreditAttribution: Salah Messaoud commentedComment #36
blainelang CreditAttribution: blainelang commentedRepresenting Toronto Sprint, we have reviewed the patch. It applies clean, we did not notice any other un-used variables. Comment #30 indicated PHPStorm was used to run a report for any more un-used variables and we did not see any other issues.
Comment #37
XanoThe latest patch needs a re-roll, because it no longer applies.
Comment #38
Salah Messaoud CreditAttribution: Salah Messaoud commentedComment #39
martin107 CreditAttribution: martin107 commented+1 looks good to me.
No funny business each line just straight forward removal of unwanted variables
Comment #40
longwaveIn some of these it looks like we could be a bit smarter:
why not:
and similarly:
looks like it could become:
Comment #41
dawehnerLet us wait until #2107533: Remove {menu_router} is in.
Comment #42
longwavePostponed on #2107533: Remove {menu_router}
Comment #43
longwaveComment #44
miraj9093 CreditAttribution: miraj9093 commentedLooks good to me..
Comment #45
dawehnerNote: #2207893: Convert menu tree building to a service. will conflict with this and also fix it.
Comment #46
star-szrThen this is a dupe (or whatever else you prefer under Closed) IMO.