Comments

neochief’s picture

Assigned:Unassigned» neochief
neochief’s picture

Status:Active» Needs review
StatusFileSize
new743 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch performance-2002726.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
neochief’s picture

StatusFileSize
new2.76 KB
FAILED: [[SimpleTest]]: [MySQL] 55,584 pass(es), 2 fail(s), and 5,310 exception(s).
[ View ]

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:

<?php
    $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.

<?php
    $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
StatusFileSize
new2.77 KB
FAILED: [[SimpleTest]]: [MySQL] 56,116 pass(es), 2 fail(s), and 5,336 exception(s).
[ View ]

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
StatusFileSize
new186.84 KB
new2.77 KB
FAILED: [[SimpleTest]]: [MySQL] 58,055 pass(es), 2 fail(s), and 5,418 exception(s).
[ View ]
new161.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
StatusFileSize
new285 bytes
new2.76 KB
PASSED: [[SimpleTest]]: [MySQL] 57,611 pass(es).
[ View ]

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

StatusFileSize
new2.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.other_.2002726-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

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
StatusFileSize
new2.18 KB
PASSED: [[SimpleTest]]: [MySQL] 59,099 pass(es).
[ View ]
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

starting work

Salah Messaoud’s picture

StatusFileSize
new978 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,330 pass(es).
[ View ]

patch re-rolled to the latest version

Salah Messaoud’s picture

Status:Needs work» Needs review
StatusFileSize
new978 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,245 pass(es).
[ View ]
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

Cottser’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
StatusFileSize
new3.7 KB
new3.57 KB
PASSED: [[SimpleTest]]: [MySQL] 59,394 pass(es).
[ View ]
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
StatusFileSize
new2.92 KB
PASSED: [[SimpleTest]]: [MySQL] 64,721 pass(es).
[ View ]
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:

<?php
-  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:

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

and similarly:

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

looks like it could become:

<?php
 
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
longwave’s picture

Status:Postponed» Needs review
Issue tags:-Needs reroll
StatusFileSize
new2.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,405 pass(es).
[ View ]
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.

Cottser’s picture

Status:Fixed» Closed (duplicate)

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