See meta #2002650: [meta, no patch] 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)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Froelund’s picture

Assigned: Unassigned » Froelund

Working on it.

Froelund’s picture

Status: Active » Needs review
FileSize
1.39 KB
leanderl’s picture

Assigned: Froelund » Unassigned
FileSize
2.6 KB

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

neochief’s picture

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

leanderl’s picture

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.

neochief’s picture

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.

leanderl’s picture

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

connorwk’s picture

Status: Needs work » Needs review
drupalmonkey’s picture

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

connorwk’s picture

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.

kerasai’s picture

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

#10 looks good, resubmitting to testbot.

kerasai’s picture

Status: Needs work » Needs review

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

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Looking good

alexpott’s picture

Title: Improve performance by removing unused local variables - core/includes/theme.inc » Improve 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.