Optimierung (und Bug ?) in /admin/modules.php

[noRiddle]

• Sortierung:
  Wenn man z.B. eine Klassenerweiterung baut und den diversen Einstellungsmöglichkeiten eine Sortierung gibt (Feld sort_order in DB-Tabelle configuration) wird diese bei der Darstellung im Edit-Modus nicht berücksichtigt. Statt dessen wird die Reihenfolge im Array das die Methode keys() zurückgibt als Sortierung verwendet (Funktion get_module_info()).
  Das ist nicht nur kontra-intuitiv sondern das DB-Feld sort_order wird auch ad absurdum geführt weil es keine Wirkung hat.
  Das würde ich als eine Art Bug bezeichnen wollen.
• Query:
  Die Query in o.g. Funktion get_module_info() wird innerhalb des for-Loops und somit mehrfach ausgeführt, was bei vielen Konfigurations-Parameteren sicherlich performance-technisch nicht optimal ist.
  Man könnte die Query, um sort_order ergänzt, außerhalb eines Loops mittels
  WHERE configuration_key IN('".implode('\',\'', $module_keys)."') ORDER BY sort_order ASC");
  machen und in einer while-Loop dann das Array $module_info füllen.

Mein Vorschlag inkl. auskommentiertem alten Code:

  function get_module_info($module) {
    $module_info = array('code' => $module->code,
                         'title' => $module->title,
                         'description' => $module->description,
                         'extended_description' => isset($module->extended_description) ? $module->extended_description : '',
                         'status' => $module->check());
    $module_info['properties'] = isset($module->properties) ? $module->properties : array();
    $module_keys = method_exists($module,'keys') ? $module->keys() : array();
    $keys_extra = array();
    
    $key_value_query = xtc_db_query("SELECT configuration_key,
                                            configuration_value,
                                            sort_order,
                                            use_function,
                                            set_function
                                       FROM " . TABLE_CONFIGURATION . "
                                      WHERE configuration_key IN('".implode('\',\'', $module_keys)."')
                                   ORDER BY sort_order ASC");
    
    while($key_value = xtc_db_fetch_array($key_value_query)) {
        if ($key_value['configuration_key'] !='') {
            $keys_extra[$key_value['sort_order']]['title'] = constant(strtoupper($key_value['configuration_key'] .'_TITLE'));
        }
        $keys_extra[$key_value['sort_order']]['value'] = $key_value['configuration_value'];
        if ($key_value['configuration_key'] !='') {
            $keys_extra[$key_value['sort_order']]['description'] = constant(strtoupper($key_value['configuration_key'] .'_DESC'));
        }
        $keys_extra[$key_value['sort_order']]['use_function'] = $key_value['use_function'];
        $keys_extra[$key_value['sort_order']]['set_function'] = $key_value['set_function'];
    }
    
    /*for ($j = 0, $k = sizeof($module_keys); $j < $k; $j++) {
      $key_value_query = xtc_db_query("SELECT configuration_key,
                                              configuration_value,
                                              use_function,
                                              set_function
                                         FROM " . TABLE_CONFIGURATION . "
                                        WHERE configuration_key = '" . $module_keys[$j] . "'");
      $key_value = xtc_db_fetch_array($key_value_query);
      if ($key_value['configuration_key'] !='') {
        $keys_extra[$module_keys[$j]]['title'] = constant(strtoupper($key_value['configuration_key'] .'_TITLE'));
      }
      $keys_extra[$module_keys[$j]]['value'] = $key_value['configuration_value'];
      if ($key_value['configuration_key'] !='') {
        $keys_extra[$module_keys[$j]]['description'] = constant(strtoupper($key_value['configuration_key'] .'_DESC'));
      }
      $keys_extra[$module_keys[$j]]['use_function'] = $key_value['use_function'];
      $keys_extra[$module_keys[$j]]['set_function'] = $key_value['set_function'];
    }*/
    $module_info['keys'] = $keys_extra;
    
    return $module_info;
  }

Damit hätten wir dem Feld sort_order wieder zur Geltung verholfen und die DB-Query optimiert.

Gruß,
noRiddle

[noRiddle]

*NACHTRAG*

Das Gesagte gilt auch für die /admin/module_export.php .

Gruß,
noRiddle

[Gerhard Waldemair]

das würde aber bedeuten, dass die vorgegebene Sortierung im keys array nicht mehr greift und das ist nicht optimal.

[noRiddle]

Naja, wo aber steht, daß die Reihenfolge im keys-Array entscheidend ist ?
(...und wieso "vorgegebene" ... und wieso "nicht mehr", wann war das so ?)
Wofür ist den das configuration-Feld sort_order ?

Ich meine, okay, wenn man's weiß, die sort_order wirkt halt bei den Modulen allgemein (was ich noch nicht mal geprüft habe)), aber eben nicht bei den Konfigurations-Werten eines Modules. Kann man drüber streiten. Aber das Feld sort_order ist nun mal da.

Die Query-Optimierung halte ich jedenfalls für gut. :-)

Gruß,
noRiddle

[Gerhard Waldemair]

Ich habe mir den COde nochmal im Detail angeschaut.
Ich sehe darin mehrere Probleme.

• alle Module bei denen keine Sortierung gepflegt wurde funktionieren nicht mehr, da die Reihenfolge verwendet wird als Index für das Array.
• auch für die Zukunft finde ich diese Lösung als gefährlich, denn eine Sortierreihenfolge muss dann unique sein
• dei der SQL Optimierung sehe ich den Vorteil der Performance, aber auch das Problem, dass die Reihenfolge nicht mehr stimmt.

Das Ticket wird deshalb vorerst nicht berücksichtigt.

[anonymous]

Nicht gepflegte Sortierung bei Modulen ist ein Argument.

Die Query lässt sich allerdings trotzdem optimieren.
Du lässt das ORDER BY in meiner Query weg und um an den Array-Key der einzelnen Values in $module_keys zu kommen, also das was du im for-Loop mit j machst, machst du zusätzlich zu diesem

$module_keys = method_exists($module,'keys') ? $module->keys() : array();

das

$module_keys = method_exists($module,'keys') ? $module->keys() : array();
$keys_module = array_flip($module_keys);

und kannst dann im while-Loop so definieren

$keys_extra[$module_keys[$keys_module[$key_value['configuration_key']]]]['title'] = constant(strtoupper($key_value['configuration_key'] .'_TITLE'));

oder von mir aus aus Gründen der Lesbarkeit so

$k = $keys_module[$key_value['configuration_key']];
$keys_extra[$module_keys[$k]]['title'] = constant(strtoupper($key_value['configuration_key'] .'_TITLE'));

Damit könnte die Query außerhalb des for-Loops laufen.

Gruß,
noRiddle

[noRiddle]

GTB, verfolgst du das hier noch ?
Was hältst du von meinem letzten Vorschlag Queries in einem Loop zu machen ?

Gruß,
noRiddle

[noRiddle]

Korrektur: Ich meinet natürlich "zu vermeiden Queries in einem Loop zu machen..."

[Gerhard Waldemair]

In 12967:

fix #1852 - optimized code in /admin/module_export.php & /admin/modules.php