Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neochief’s picture

Assigned: Unassigned » neochief
neochief’s picture

Status: Active » Needs review
FileSize
743 bytes
neochief’s picture

FileSize
2.76 KB

Ok, here's the better one.

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, performance-2002726.patch, failed testing.

neochief’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work

The last submitted patch, performance-2002726.patch, failed testing.

kerasai’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +Novice

The last submitted patch, performance-2002726.patch, failed testing.

kerasai’s picture

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

    $menu = reset(menu_router_build());

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.

    $menu = menu_router_build();
    $menu = reset($menu);
YesCT’s picture

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

somepal’s picture

Status: Needs work » Needs review
FileSize
2.77 KB

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

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, 2002726_unused-local-var_11.patch, failed testing.

somepal’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +Novice

The last submitted patch, 2002726_unused-local-var_11.patch, failed testing.

martin107’s picture

Category: bug » task
FileSize
186.84 KB
2.77 KB
161.05 KB

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

martin107’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, new.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
285 bytes
2.76 KB

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

YesCT’s picture

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)

MichaelLund’s picture

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

MichaelLund’s picture

Issue summary: View changes

Updated issue summary.

parthipanramesh’s picture

Issue summary: View changes
Status: Needs review » Needs work

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

royal121’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 20: drupal8.other_.2002726-20.patch, failed testing.

royal121’s picture

Status: Needs work » Needs review
FileSize
2.18 KB
areke’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

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.

xjm’s picture

Title: Improve performance by removing unused local variables - core/includes/menu.inc » Remove unused local variables from core/includes/menu.inc
Salah Messaoud’s picture

Assigned: neochief » Salah Messaoud
Issue tags: +TUNIS_2013_DECEMBER

starting work

Salah Messaoud’s picture

patch re-rolled to the latest version

Salah Messaoud’s picture

Status: Needs work » Needs review
FileSize
978 bytes
elgordogrande’s picture

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?

parthipanramesh’s picture

Status: Needs review » Reviewed & tested by the community

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

star-szr’s picture

Issue tags: -Needs reroll
areke’s picture

Status: Reviewed & tested by the community » Needs work

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

Salah Messaoud’s picture

Status: Needs work » Needs review
FileSize
3.7 KB
3.57 KB
Salah Messaoud’s picture

Assigned: Salah Messaoud » Unassigned
blainelang’s picture

Status: Needs review » Reviewed & tested by the community

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.

Xano’s picture

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

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

Salah Messaoud’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.92 KB
martin107’s picture

+1 looks good to me.

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

longwave’s picture

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) {
dawehner’s picture

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

longwave’s picture

Status: Needs review » Postponed
miraj9093’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me..

dawehner’s picture

Status: Reviewed & tested by the community » Fixed

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

star-szr’s picture

Status: Fixed » Closed (duplicate)

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