Remove unused local variables from core/includes/menu.inc

Ok, here's the better one.

#3: performance-2002726.patch queued for re-testing.

I'm not sure. Lets see if we can get some eyes on this.

although array_shift() also uses reset internally, lets find out if test succeeds.

#11: 2002726_unused-local-var_11.patch queued for re-testing.

Please 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

this 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);

would 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)

Re-rolling patch in comment #18, since it never got committed

Updated issue summary.

Sorry but the latest patch needs work. I can't apply this patch without errors..

20: drupal8.other_.2002726-20.patch queued for re-testing.

This 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.

starting work

patch re-rolled to the latest version

Prior 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?

EDIT: Now I also think that @elgordogrande is right. Sorry

@elgordogrande is right. These other unused variables should also be taken care of in this patch.

Representing 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.

The latest patch needs a re-roll, because it no longer applies.

+1 looks good to me.

No funny business each line just straight forward removal of unwanted variables

In some of these it looks like we could be a bit smarter:

-  foreach ($tree as $key => $v) {
+  foreach (array_keys($tree) as $key) {
     if ($tree[$key]['link']['router_path'] == 'node/%') {
...
        $tree[$key]['link']['access'] = FALSE;

why not:

  foreach ($tree as $key => &$item) {
     if ($item['link']['router_path'] == 'node/%') {
...
        $item['link']['access'] = FALSE;

and similarly:

-  foreach ($menu as $path => $v) {
+  foreach (array_keys($menu) as $path) {
     $item = &$menu[$path];

looks like it could become:

  foreach ($menu as $path => &$item) {

Let us wait until #2107533: Remove {menu_router} is in.

Postponed on #2107533: Remove {menu_router}

Looks good to me..

Note: #2207893: Convert menu tree building to a service. will conflict with this and also fix it.

Then this is a dupe (or whatever else you prefer under Closed) IMO.