[tags_articles] 1.2.0 stable Build 24 vs Build 23: 1 Bug, 1 fehlendes Feature

[deerwood]

Moin Helmut,

von Build 23 auf Build 24 hast Du 2 SQL Statements innerhalb der foreach-Schleife weg optimiert. An sich sehr löblich, vor allem aus Performance-Sicht.

Aber leider hat sich dabei ein Bug in der jetzigen Zeile 211 von ModuleTaggedArticleList.php eingeschlichen (else Zweig, dort wo {{link::*}} und {{link_url::*}} erzeugt werden). Die beiden InsertTags benötigen natürlich eine PAGE ID und haben sie in Build 23 auch bekommen, jetzt fügst Du dort aber die ARTCLE ID ein ... und damit passen dann Teaser/Html nicht mehr zu den erzeugten Links.

Außerdem hatte Build 23 in dieser Zeile und in Zeile 207 (dem if Teil) das "data" Array mit Inhalten gefüllt, insbesondere die Page Id und die Article Id, davon habe ich in einem angepassten Template auch Gebrauch gemacht. Build 24 liefert jetzt aber ein leeres "data" Array. Da Du die Artikel Id ja sowieso hast und die Page Id wegen des Bugs auch benötigt wird, wäre es schön, wenn Du im Zuge der Korrektur das "data" Array wieder mit diesen beiden Ids befüllen würdest.

Bezüglich der performanten Beschaffung der Page Id: die Page Id ist doch tl_article.pid, Du könntest doch in ModuleTaggedArticleList::getArticlesForPages() die pid mit selektieren und in Zeile 100 ein Array mit id und pid in $this->arrArticles pushen, dann hättest Du die Page Id praktisch kostenlos an der Hand?

LG, Georg

[deerwood]

Moin alle,

da ich [tags_articles] wirklich brauche, habe ich mich mal um die bereits geschilderten Probleme gekümmert. Bei der Umsetzung ist auch noch ein weiterer Bug in Build 24 aufgetaucht: sobald man die Template-Variablen 'teaser' bzw. 'html' nutzt UND die Seite, auf der das Modul ausgegeben wird, ebenfalls erzeugt werden muss, geht Contao in eine rekursive Schleife, die eigentlich in ModuleTaggedArticleList::compile() mit einem Lock verhindert werden soll.

Das liegt offenbar daran, dass die in B24 neu eingeführten Inserttags {{insert_article::*}} und {{article_teaser::*}} von Contao erst ersetzt werden NACHDEM die Methode compile() durchgelaufen ist. Damit ist dann das Locking in compile() unwirksam.

Hier ein DIFF des BUILD24 vs. meiner Version, Erläuterungen unterhalb:

--- system/modules/tags_articles/ModuleTaggedArticleList.php.BUILD24    Di 13. Mrz 10:01:14 2012
+++ system/modules/tags_articles/ModuleTaggedArticleList.php    Do 29. Mrz 04:07:35 2012
@@ -37,7 +37,7 @@
         if (TL_MODE == 'BE')
         {
             $objTemplate = new BackendTemplate('be_wildcard');
-            $objTemplate->wildcard = '### GLOBAL ARTICLE LIST ###';
+            $objTemplate->wildcard = '### TAGGED ARTICLE LIST ###';
 
             return $objTemplate->parse();
         }
@@ -71,14 +71,31 @@
 
             // Get published articles
             $pids = join($this->arrPages, ",");
+
+            $order_by = 'title ASC';
+            if (isset($GLOBALS['MISC']['tag_articles_id'][$this->id]['ORDER_BY_START'])
+                && in_array($GLOBALS['MISC']['tag_articles_id'][$this->id]['ORDER_BY_START'], array('ASC', 'DESC')))
+            {
+                // Use the 'start' field (Anzeigen ab/Show from) for sorting (interpret it as an article creation/publishing date).
+                // Instead of a backend field in the module, look for the setting in localconfig.php.
+                // If you use this (hidden) feature you probably also might want to add the following 2 lines to dcaconfig.php,
+                // making the start field mandatory (in the DCA, not the DB).
+                //   $GLOBALS['TL_DCA']['tl_article']['fields']['start']['eval']['mandatory'] = true;
+                //   $GLOBALS['TL_DCA']['tl_article']['fields']['start']['default'] = time();
+                // Note however, after creating a new page, you still have to set the article start date explicitly, because the Contao core
+                // auto inserts an article for you and doesn't check for mandatory fields. As soon as you edit the article settings,
+                // e.g. to add tags :) you'll be forced to also fill in the 'start' field.
+                $order_by = 'start ' . $GLOBALS['MISC']['tag_articles_id'][$this->id]['ORDER_BY_START'] . ', title ASC';
+            }
+
             if ($this->show_in_column)
             {
-                $objArticles = $this->Database->prepare("SELECT id, title, alias, inColumn, cssID FROM tl_article WHERE inColumn = ? AND pid IN (" . $pids . ") " . (!BE_USER_LOGGED_IN ? " AND (start='' OR start<?) AND (stop='' OR stop>?) AND published=1" : "") . " ORDER BY sorting")
+                $objArticles = $this->Database->prepare("SELECT id, pid, title, alias, inColumn, cssID, teaser, start FROM tl_article WHERE inColumn = ? AND pid IN (" . $pids . ") " . (!BE_USER_LOGGED_IN ? " AND (start='' OR start<?) AND (stop='' OR stop>?) AND published=1" : "") . " ORDER BY " . $order_by)
                                               ->execute($this->inColumn, $time, $time);
             }
             else
             {
-                $objArticles = $this->Database->prepare("SELECT id, title, alias, inColumn, cssID FROM tl_article WHERE pid IN (" . $pids . ") " . (!BE_USER_LOGGED_IN ? " AND (start='' OR start<?) AND (stop='' OR stop>?) AND published=1" : "") . " ORDER BY sorting")
+                $objArticles = $this->Database->prepare("SELECT id, pid, title, alias, inColumn, cssID, teaser, start FROM tl_article WHERE pid IN (" . $pids . ") " . (!BE_USER_LOGGED_IN ? " AND (start='' OR start<?) AND (stop='' OR stop>?) AND published=1" : "") . " ORDER BY " . $order_by)
                                               ->execute($time, $time);
             }
             if ($objArticles->numRows < 1)
@@ -86,18 +103,40 @@
                 return;
             }
 
+            global $objPage;
+            $format = $objPage->outputFormat;
+            if (!empty($format))
+            {
+                $this->import('String');
+            }
+
             while ($objArticles->next())
             {
+                /* This code seems to be a useless "left over copy/paste" from an old version of system/modules/frontend/ModuleArticleList.php
                 // Skip first article
                 if (++$intCount == 1 && $this->skipFirst)
                 {
                     continue;
                 }
+                */
 
-                $cssID = deserialize($objArticles->cssID, true);
-                $alias = strlen($objArticles->alias) ? $objArticles->alias : $objArticles->title;
+                $objArticles->cssID = deserialize($objArticles->cssID, true);
+                // ??? $alias = strlen($objArticles->alias) ? $objArticles->alias : $objArticles->title;
+                $objArticles->startDate = (intval($objArticles->start) > 0) ? $this->parseDate($GLOBALS['TL_CONFIG']['datimFormat'], intval($objArticles->start)) : '';
+                $objArticles->teaser = $this->replaceInsertTags($objArticles->teaser);
+                if (!empty($format))
+                {
+                    if ($format == 'xhtml')
+                    {
+                        $objArticles->teaser = $this->String->toXhtml($objArticles->teaser);
+                    }
+                    else
+                    {
+                        $objArticles->teaser = $this->String->toHtml5($objArticles->teaser);
+                    }
+                }
 
-                array_push($this->arrArticles, $objArticles->id);
+                array_push($this->arrArticles, $objArticles->row());
             }
         }
     }
@@ -164,11 +203,11 @@
             }
         }
 
-        foreach ($this->arrArticles as $objArticleId)
+        foreach ($this->arrArticles as $arrArticle)
         {
             if (count($tagids) || !$this->hide_on_empty)
             {
-                if (in_array($objArticleId, $tagids) || (!$this->hide_on_empty && count($tagids) == 0))
+                if (in_array($arrArticle['id'], $tagids) || (!$this->hide_on_empty && count($tagids) == 0))
                 {
                     $objTemplate = new FrontendTemplate('taglist');
                     $objTemplate->showTags = $this->article_showtags;
@@ -192,7 +231,7 @@
                             }
                             $pageArr = $objPage->row();
                         }
-                        $tags = $this->getTags($objArticleId);
+                        $tags = $this->getTags($arrArticle['id']);
                         foreach ($tags as $id => $tag)
                         {
                             $strUrl = ampersand($this->generateFrontendUrl($pageArr, $items . '/tag/' . str_replace(' ', '+', $tag)));
@@ -204,11 +243,11 @@
 
                     if ($this->linktoarticles)
                     { // link to articles
-                        $articles[] = array('content' => '{{article::' . $objArticleId . '}}', 'url' => '{{article_url::' . $objArticleId . '}}', 'tags' => $taglist, 'data' => array(), 'html' => '{{insert_article::' . $objArticleId . '}}', 'teaser' => '{{article_teaser::' . $objArticleId . '}}');
+                        $articles[] = array('content' => '{{article::' . $arrArticle['id'] . '}}', 'url' => '{{article_url::' . $arrArticle['id'] . '}}', 'tags' => $taglist, 'data' => $arrArticle, 'html' => $this->getArticle($arrArticle['id'], false, true), 'teaser' => $arrArticle['teaser']);
                     }
                     else
                     { // link to pages
-                        $articles[] = array('content' => '{{link::' . $objArticleId . '}}', 'url' => '{{link_url::' . $objArticleId . '}}', 'tags' => $taglist, 'data' => array(), 'html' => '{{insert_article::' . $objArticleId . '}}', 'teaser' => '{{article_teaser::' . $objArticleId . '}}');
+                        $articles[] = array('content' => '{{link::' . $arrArticle['pid'] . '}}', 'url' => '{{link_url::' . $arrArticle['pid'] . '}}', 'tags' => $taglist, 'data' => $arrArticle, 'html' => $this->getArticle($arrArticle['id'], false, true), 'teaser' => $arrArticle['teaser']);
                     }
                 }
             }

Änderungen, sortiert nach Wichtigkeit:

1. FIX: link to pages now uses the page id instead of the article id
2. FIX: no recursion, when 'html' or 'teaser' is used in the template (for the page with the module)
3. FEATURE: populatet the 'data' array again; note several differences/enhancements to BUILD23
4. FEATURE/FIX: default order is by 'title ASC'
5. FEATURE: optionally sort by start date, then by title (see comments in the source)

Wie Ihr seht, habe ich in getArticlesForPages() am Ende der Schleife die komplette Artikel-DB-Zeile weitergereicht (nicht nur die ID). Damit stehen alle essentiellen Felder später zur Verfügung.

Im Anhang das ZIP mit meiner Version von ModuleTaggedArticleList.php, damit Ihr das erproben bzw. mit Euren DIFF Tools vergleichen könnt.

LG, Georg

[hschottm]

Ach Georg,

du bist ein Schatz!
Ich gebe zu, ich habe das Performance-'Update' etwas schnell rausgehauen, es ging vorwiegend darum, dass jemand ein Problem hatte, weil er mehrere hundert Artikel mit Tags versehen hatte und der Aufruf der Listen über die Tag Cloud ewig gedauert hat.
Als ich dann da so beim herumoptimieren über das data-Array gestolpert bin und mich fragte, warum ich das eigentlich mal eingeführt habe, dachte ich mir, dass das wahrscheinlich ohnehin niemand verwendet, weil die meisten Daten darin ja auch anderweitig zur Verfügung stehen.
Tja, a) nicht gut gedacht und b) nicht richtig gedacht und c) dann auch noch andere Fehler eingestreut.
Ich finde deine Wiedereinführung, die Bugfixes und die Adaption mit der Konfiguration über DCA sehr schön und habe das auch so in den neuen Build übernommen.
Bitte entschuldige dieses Hotfixing. Leider hab ich auch mal wieder nicht die Zeit, mich um die Forenbeiträge zu kümmern, deswegen war es sehr gut, dass du mich direkt angeschrieben hast...

Gruß,
Helmut