See meta #2002650: [meta] improve maintainability by removing unused local variables

core/includes/theme.inc

  • Unused local variable $themes (line 746)
  • Unused local variable $number (line 2860)
  • Unused local variable $i (line 2201)
  • Unused local variable $number (line 2264)
  • Unused local variable $user (line 2674)
Files: 
CommentFileSizeAuthor
#10 8.x-unused-vars-2002730-9.patch1.69 KBdrupalmonkey
PASSED: [[SimpleTest]]: [MySQL] 55,966 pass(es).
[ View ]
#3 removing-unused-local-variables-2002730.patch2.6 KBleanderl
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#2 8.x-remove-unused-local-variables-2002730-2.patch1.39 KBFroelund
PASSED: [[SimpleTest]]: [MySQL] 55,870 pass(es).
[ View ]

Comments

Assigned:Unassigned» Froelund

Working on it.

Status:Active» Needs review
StatusFileSize
new1.39 KB
PASSED: [[SimpleTest]]: [MySQL] 55,870 pass(es).
[ View ]

Assigned:Froelund» Unassigned
StatusFileSize
new2.6 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Removed the variables -- couldn't find $number on row 2860: "Unused local variable $number (line 2860)". Also checked on row 1860 and 860.

#2 looks better for me (there's weird code block removal in #3 + no new line removal)

What does "+ no new line removal" mean? Would be nice if you could be more specific. Is there a new line somewhere that should be removed? I would love to learn to be a better patch contributor.

The code block that was removed was as far as I could tell relating to the deleted variable and could/should thus be deleted. But of course I could be wrong.

No new line removal means the line after "- global $user;"

As for the block of code, I think it's not useless. You should have removed just a "$themes = array();" as this line is useless, since the variable will be reassigned later anyway (see the if-else statements).

Status:Needs review» Needs work

The last submitted patch, removing-unused-local-variables-2002730.patch, failed testing.

@neochief, yes it turns out you were right about the code block. It should stay in.

Status:Needs work» Needs review

StatusFileSize
new1.69 KB
PASSED: [[SimpleTest]]: [MySQL] 55,966 pass(es).
[ View ]

Rerolled the patch from #2, removed the other unnecessary $number variable from line ~2180ish.

Status:Needs review» Needs work

+++ b/core/includes/theme.inc
@@ -743,45 +743,8 @@ function list_themes($refresh = FALSE) {
-    if (!defined('MAINTENANCE_MODE')) {
-      try {
-        $themes = system_list('theme');
-      }
-      catch (Exception $e) {
-        // If the database is not available, rebuild the theme data.
-        $themes = _system_rebuild_theme_data();
-      }
-    }
-    else {
-      // Scan the installation when the database should not be read.
-      $themes = _system_rebuild_theme_data();
-    }
-
-    foreach ($themes as $theme) {
-      foreach ($theme->info['stylesheets'] as $media => $stylesheets) {
-        foreach ($stylesheets as $stylesheet => $path) {
-          $theme->stylesheets[$media][$stylesheet] = $path;
-        }
-      }
-      foreach ($theme->info['scripts'] as $script => $path) {
-        $theme->scripts[$script] = $path;
-      }
-      if (isset($theme->info['engine'])) {
-        $theme->engine = $theme->info['engine'];
-      }
-      if (isset($theme->info['base theme'])) {
-        $theme->base_theme = $theme->info['base theme'];
-      }
-      // Status is normally retrieved from the database. Add zero values when
-      // read from the installation directory to prevent notices.
-      if (!isset($theme->status)) {
-        $theme->status = 0;
-      }
-      $list[$theme->name] = $theme;

This is removing more then expected.

@connork's comment in #11 is referring to the patch submitted in #2.

#10 looks good, resubmitting to testbot.

Status:Needs work» Needs review

#10: 8.x-unused-vars-2002730-9.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Looking good

Title:Improve performance by removing unused local variables - core/includes/theme.incImprove code maintainability by removing unused local variables - core/includes/theme.inc
Status:Reviewed & tested by the community» Fixed

Committed abbd5be and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.