Store Locator ( locator ) – Finger weg! Warum?

Die Extension Store Locator ( locator ) taucht immer wieder (zumindest im deutschsprachigen Raum) als Empfehlung für Umkreissuchen auf. D.h. wenn es drum geht, Filialen, Händler odgl. in einem gewissen Umkreis zu suchen, ausgehend von einer vom bestimmten User bestimmten Position.

Warum man aber tunlichst die Finger von der Extension lassen sollte, hat mir das vergangene Wochenende gezeigt….

Leichtes Warmwerden

Begonnen hatte das Eintauchen in den Code des Store Locator mit dem Hinweis einer Kundin, dass auf ihrer Website keine Orte mit scharfem S gefunden werden bzw. nur, wenn man dieses als „ss“ eingibt. Der Weg führte mich dann durch den PHP-Code bis zu den Abfragen der Geopositioning-Dienste, die in der Datei pi1/class.tx_locator_model.php zu finden sind. Hier kam dann der Schock:

1. Coding Guidelines existieren nicht bzw. werden ignoriert
2. Umgang mit Zeichensätzen ist willkürlich
3. Konfigurationsvariablen von Extension und TYPO3 werden teils ignoriert bzw. hart überschrieben
4. Geopositioning-Dienste werden abgefragt, die ihren Dienst (in der API-Version) eingestellt haben
5. 40 hidden-Fields statt einem DB-Requests
6. SQL-Queries werden zusammengebaut, die UIDs vermischen und Werte überschreiben
7. Missachtung der enablecolumns/enableFields
8. TYPO3-API ist in weiten Teilen anscheinend unbekannt
9. Vermischung der MVC-Einheiten
10. Logische Fehler

Keine Coding Guidelines

Die Formatierung des Programmcodes ist stellenweise vorhanden, größtenteils aber eher willkürlich. Dies trägt nicht sonderlich zur Lesbarkeit und zum Verständnis des Codes bei.
Die Benamung von Methoden und Variablen ist ebensfalls willkürlich, mal in CamelCase, mal als lowercase-with-underscore, und folgt keinem einheitlichen Schema.
Eine Dokumentation des Codes (Kommentare) insbesondere auch der Methode fehlt nahezu komplett. Da macht das Bugsuchen Freude, wenn man erst einmal die Logik, Zusammenhänge und Methoden analysieren darf.

Ein gemeinsames Schema lässt sich nicht erkennen. Die Empfehlungen/Vorgaben der TYPO3-Coding Guidelines werden nicht eingehalten.

Willkürlicher Umgang mit Zeichensätzen

TYPO3 bietet zahlreiche Konfigurationswerte, die in Extensions herangezogen werden können, um Daten (seien es interne aus der Datenbank oder Usereingaben) entsprechend ihrer Zeichensatzcodierung zu behandeln, zu konvertieren und passend mit anderen Systemen (z.B. den APIs von Geopositioning-Diensten) auszutauschen.

Diese Extension scheint jedoch auf spezielle Anforderung einiger (Mit-)Entwickler optimiert zu sein und keinesfalls allgemein nutzbar. Teilweise erfolgt ein utf8_encode() auf die Usereingaben, teilweise nicht. Manchmal wird die Rückantwort der Dienste konsequenterweise mit utf8_decode() behandelt, teilweise auch nicht. In welchem Zeichensatz die Usereingaben erfolgen bzw. in welchem Zeichensatz die Dienste antworten, das bleibt unberücksichtigt. So ist es eher Zufall, dass der Code für ein konkretes Projekt funktioniert.

Konkret wird in den Zeilen 250 (Sonderfall Polen) und 511 (Fallback-Fall der internationalen Abfrage) die Variable $apiURL encodiert. Warum dies nur in diesen beiden Fällen geschieht, und warum im internationalen Fall auch nur für eine der Fallbackvarianten, erklärt sich mir nicht.

Die meisten APIs der Gegenstellen erwarten Strings als urlencodierten UTF-8-Wert. Korrekterweise müsste also jeder String, der in die Aufruf-URL der APIs eingebunden zum einen in UTF-8 gewandelt werden (falls erforderlich), zum anderen durch urlencode() geschickt werden. Für das UTF-8-Encoding sollte dabei nicht uft8_encode() herangezogen werden, da dies voraussetzen würde, dass der Input in ISO-8859-1 erfolgt (PHP-Doku uft8_encode – „Konvertiert eine ISO-8859-1-Zeichenkette in UTF-8“). Eine passendere Methode bietet hier die TYPO3-API: t3lib_cs::utf8_encode – „Converts $str from $charset to UTF-8“.

Unter der Annahme, dass $params ein Array mit Key-Value-Paaren für die Parameter der API-URL ist, sollte nachfolgender Code die Zeichensatzcodierung korrekt umsetzen:

foreach ($params as $key => $value) {
  if ($GLOBALS['TSFE']->metaCharset != 'utf-8') {
     $value = t3lib_cs::utf8_encode($value, $GLOBALS['TSFE']->metaCharset);
  }
  $apiURL .= $key . '=' . urlencode($value) . '&';
}

Konfigurationsvariablen werden ignoriert

Die Nicht-Beachtung der Konfigurationswerte im Umgang mit Zeichensätzen wurde im vorherigen Abschnitt schon leicht geschrammt. TYPO3 stellt so vielfältige Werte bereit, die einem eigentlich den Umgang mit Zeichensätzen in der Datenbank, dem DB-Connector, dem Rendering und der FE-Ausgabe erleichtern (renderChartset, metaCharset). Warum diese nicht genutzt werden und stattdessen fest von ISO-8859-1-Zeichenketten ausgegangen wird, ist mir unverständlich.

Aber auch an anderer Stelle werden die Konfigurationswerte missachtet: Via FlexForm lässt sich eigentlich für die Extension konfigurieren, ob zum Abruf von Webseiten (Geopositioning-APIs) auf CURL zurückgegriffen werden soll oder nicht. Die TYPO3-weite Einstellung aus $TYPO3_CONF_VARS[‚SYS‘][‚curlUse‘] wird dabei getrost ignoriert. In meinen Augen ist das ein sehr merkwürdiges Vorgehen, dass solch eine sehr technische Entscheidung durch die Eingabe im Contentelement via Flexform dem Redakteur überlassen wird (bzw. vielleicht noch dem Integrator). Eigentlich sollte dies eine Entscheidung sein, die Aufgrund der Serverumgebung vom System-Administrator oder dem TYPO3-Integrator global für die ganze TYPO3-Instanz gesetzt wird. In Redakteurshand hat die Einstellung jedenfalls nichts zu suchen, insbesondere wenn sie den globalen Vorgaben aus der localconf/Install-Tool überschreibt bzw. diese ignoriert.

Aktualität bzw. Versionen der Geopositioning-Dienste

Um schließlich festzustellen, warum die Geopositionen von Orten mit scharfem „S“ in der Installation nicht gefunden wurden, war eine der ersten Debug-Quellen die Ausgabe der Anfragen an die Geopositioning-Dienste. Zunächst fiel hier auf, dass Anfragen nacheinander an unterschiedliche Dienste gesendet werden, was laut Programmcode nur der Fall sein sollte, wenn ein Request kein Ergebnis liefert. D.h. hier lieferten wohl mehrere Dienste keine Ergebnisse.

Anhand der generierten Requests bin ich dem nachgegangen und habe händisch die Dienste abgefragt – und interessante Informationen dabei herausgefunden:

• Der verwendete Yahoo MapService wird zum Ende des Jahres eingestellt. Er sollte daher zeitnah gegen den Nachfolger, Yahoo PlaceFinder, ersetzt werden (hierfür wird ein API-Key erforderlich).
• Der verwendete Google Geocoding-Dienst wird in veralteter Version verwendet. Auch hier wäre ein Update fällig.
• Der dritte für den allgemeinen Fall verwendete Dienst, localsearchmaps, unterstützt inzwischen als Antwort neben einen Javascript-String auch das XML-Format. Da in der vorliegenden Extension das Ergebnis nicht direkt in Javascript eingebunden wird, wäre hier ein Wechsel sinnvoll.

Somit scheint die Abfrage bei drei von drei Diensten eine Überarbeitung zu benötigen, um zukunftssicherer zu sein.

Die weiteren Dienste (v.a. Sonderfälle für USA, Kanada,…) hatte ich nicht näher untersucht, dies wäre aber wohl auch anzuraten, sollte man EXT:locator für die Sonderfall-Länder einsetzen wollen.

Unzählige hidden-Fields

Sehr merkwürdig wirkt die Behandlung von Daten über die Stores. Sind sie einmal aus der Datenbank abgerufen, basiert deren weitere Nutzung auf der Weitergabe via versteckter Formularfelder. Vielleicht waren das ursprünglich nicht viele Werte, sodass es vertretbar war. Warum aber inzwischen über 40 (vierzig!) hidden-Fields genutzt werden, statt anhand der UID des entsprechenden Datensatzes noch einmal die Daten neu auf der nächsten Seite abzufragen, erschließt sich mir eigentlich nicht…

Mehrdeutige Schlüssel in SQL-Queries

…eigentlich. Denn eine mögliche Ursache fand sich dann doch.

Statt für Detailansichten stets die ellenlange Liste an Parametern mitzugeben, suchten wir eine einfachere Lösung. Jeder Datensatz hat üblicherweise einen Identifier, in TYPO3 in der Regel mit Attribut uid in gleichnamiger Spalte. Auf Basis dieser wollten wir Details abrufen (Wir hatten die Tabelle erweitert, d.h. brauchten im Detailview unsere zusätzlichen Spalten, die nicht in den hidden-Fields steckten). Aus irgendeinem Grund kam nichts Sinnvolles heraus, stets war es ein Rätsel, wie die Extension auf die UID des Datensatzes kam.

Im Code fanden sich dann in der Model-Klasse (Zeile 1039) folgende Parameter für die SQL-Abfrage der Stores:

$table = 'tx_locator_locations a, tx_locator_categories b';
$fields = " distinct *, a.uid as storeuid, imageurl as image, (((acos(sin(($lat*$pi/180)) * sin((lat*$pi/180)) + cos(($lat*$pi/180)) *  cos((lat*$pi/180)) * cos((($lon - lon)*$pi/180)))))*6370) as distance";

Was bitte soll der Stern-Selector in einer Query, die über zwei Tabellen geht, ohne eine Tabelle dafür zu spezifizieren??? Die beiden Tabellen beinhalten auch die TYPO3-Standardspalten, d.h. im Result werden die Werte durch uneindeutige Keys nichtssagend für: uid, pid, parentuid, tstamp, crdate, cruser_id, sorting, hidden, deleted, sys_language_uid, l10n_parent, l10n_diffsource, fe_group – oder andersherum ausgedrückt: außer tx_locator_categories.name und tx_locator_categories.description sind die Spalten der Kategorietabelle somit unnütz.

Dass die Problematik einem Entwickler aufgefallen ist, ist erkennbar an der Vergabe eines Aliases (a.uid as storeuid). Der Alias-Name verwirrt jedoch auch etwa, da in der Tabelle auch schon eine storeid („id“ statt „uid“) existiert. Warum statt des Aliases und somit dem dritten Identifier (uid, storeid, storeuid) nicht die Query in ihrer Form hinterfragt und bereinigt wurde – keine Ahnung.

Missachtung des TCA (enablecolumns)

Im TCA gibt es den Key enablecolumns, dessen Nutzen in der Core API wie folgt erklärt wird:

Specifies which publishing control features are automatically implemented for the table.
This includes that records can be „disabled“ or „hidden“, have a starting and/or ending time
and be access controlled so only a certain front end user group can access them.

Im TCA der Extension wird in der Datei ext_tables.php hiervon auch Gebrauch gemacht. Es werden hidden, endtime und fe_group für die Locations-Tabelle als enablecolumns definiert (Warum starttime nicht existiert ist eine andere Frage…). Für die Kategorie-Tabelle sind es die Spalten hidden und fe_group.

Nun werfen wir einen Blick in die Model-Klasse. Findet irgendwer „endtime“ in einer der Queries? Oder den Aufruf der Methode enableFields(), die die im TCA definierten enablecolumns berücksichtigen würde? Ich fand nichts. Wozu ist das Feld überhaupt existent in der Extension, wenn es eh missachtet wird??? Wozu ist sowohl in den Locations als auch in den Kategorien die fe_group-Spalte existent und in den enablecolumns angegeben, wenn sie ebenfalls nicht beachtet wird?

Nicht-Nutzung der TYPO3-API

Die Missachtung des TCA könnte daher kommen, dass es etwas umständlich scheint, jedesmal an all die enablecolumns-Bedingungen zu denken, und diese in WHERE-Klauseln zu packen (v.a. für start-/endtime, fe_group). In der TYPO3-API steht hierfür jedoch eine sehr einfache Möglichkeit zur Verfügung: enableFields(). Diese Methode übernimmt für den Entwickler die Handhabung der enablecolumns und liefert einen fertigen String zum anhängen an eine WHERE-Klausel.

Aber nicht nur hier scheint die API weitgehend unbekannt zu sein bzw. die Nutzung der API für sinnvoll erachtet zu werden. So finden sich zahlreiche Aufrufe der mysql_*-Funktionen im Code, satt die Wrapper-Methoden der Klasse t3lib_DB zu verwenden, die ggf. auch die Nutzung anderer DBMS ermöglichen würde.

Vermischung der MVC-Einheiten

Positiv aufgefallen war mir schon zu Beginn der Arbeiten mit dem Code, dass die Extension auf die Trennung nach MVC aufbaut, d.h. die Datenhaltung, Layout und Logik trennt. Aber was ist das? In der Model-Klassen Aufrufe von substituteMarkerArray()? Ja, am Ende der Klassen finden sich mehrere Methoden (searchStore(), searchAllStores(), listMailStores()), die offensichtlich keine Daten zurücklifern, sondern fertig gerenderten HTML-Code. Was das mit dem Model zu tun haben soll – keine Ahnung.

Logische Fehler und Unstimmigkeiten

Bleiben wir gleich bei einer der eben erwähnten Methoden: listMailStores(). In Zeile 1649 wird in $content hartcodiert ein DIV definiert und geöffnet – die Variable wird nirgends mehr verwendet. Dafür treffen wir am Ende der Methode auf $listContent, worin sowohl ein DIV als auch eine TABLE geschlossen wird. Auch diese Variable taucht sonst nirgends mehr auf. Am Ende wird dann von der Methode $final zurückgegeben.

In Zeile 242f der Model-Klasse ist folgendes zu lesen

if ($country == "Polen") $country="pl";
if ($country == 'pl' && $address == '')

Wieso für den String „Polen“ der Wert von $country neu gesetzt wird, ist weder kommentiert, noch erschließt es sich aus dem Code auf andere Weise. $country ist im Code stets das Länderkürzel aus zwei Buchstaben. Warum für Polen (und nur für Polen) auch ein vollständiger Name als Wert akzeptiert bzw. behandelt wird, bleibt unklar. Und wenn hier schon nicht das Landeskürzel verwendet wird, dann sollten doch zumindest mögliche Sprachversionen berücksichtigt werden…

In der Methode geocode_address() des Models werden sechs Länder einer Sonderbehandlung unterzogen. So folgen hintereinander fünf IF-Blöcke. Warum nicht – wie zumindest einmal gemacht –  stattdessen IFELSE verwendet wird ist unverständlich. Eine Adresse kann logischerweise nur in einem Land liegen, d.h. egal wie der Aufruf ist, es wird immer nur eine Bedingung erfüllt sein. Man könnte sich also getrost die weiteren Prüfungen sparen, wenn bereits eine Bedingung erfüllt war.

Fazit

Während ich anfangs noch gehofft hatte, durch Patches die Extension funktionstüchtig bekommen zu können und mit der Community teilen zu können, war ich nach einem Wochenende Codedurchsuchens einfach geschockt. In der Extension ist so vieles unlogisch, unsauber und unverständlich, dass ich nicht weiß, wo mit einem Patch anzufangen wäre, und wo aufzuhören. Es scheint, als wäre die Extension gewuchert und wäre etwas außer Kontrolle geraten. Eine klare Struktur/Refactoring wäre dringend nötig.

Für diesen Artikel wurde die Extension Store Locator nicht komplett analysiert und durchgesehen. Die beschriebenen Mängel sind nur Einzelpunkte, die auf der Ursachensuche unseres Problems aufgefallen sind. Es ist leider zu erwarten, dass in den nicht näher betrachteten Teilen des Codes ähnliche Verhältnisse herrschen.

Somit werden wir wohl doch die Idee weiterverfolgen, eine eigene Umkreissuche umzusetzen, die sich auf den Kern der Funktionalität beschränkt, und die nicht durch zig Sonderfälle zur fehleranfälligen Eierlegendewollmilchsau mutiert.

Der Artikel bezieht sich auf die Extension-Version 2.4.0, die Kritikpunkte waren aber meist auch in Version 2.2.4 schon zu finden.