Refactor Link, Linklist Widgets and rex_article_slice table to support links containing anchors

[ynamite]

See also:
#6360

[ynamite]

Sorry für die vielen Commits, mir war erst gar nicht klar, dass ich das vorab lokal hätte testen können. Wieder was gelernt

[tbaddade]

Könntest du bitte Beispielcode liefern?

1. Wie erweiterst du die Linkmap über den EP.
2. Wie sieht die Moduleingabe aus
3. Wie sieht die Modulausgabe aus

Mein Test

boot.php Project AddOn

if (rex::isBackend()) {
    rex_extension::register('LINKMAP_ARTICLE_LIST', static function (rex_extension_point $ep) {
        $subject = $ep->getSubject();
        $subject .= '<li class="list-group-item"><a href="javascript:insertLink(\'redaxo://1#anker\',\'Home\u0020\u005B1\u005D\');" class="rex-offline"><i class="rex-icon rex-icon-sitestartarticle"></i> Home - Anker<span class="list-item-suffix">#</span></a></li>';
        $ep->setSubject($subject);
    });
}

Modulausgabe

$slice = $this->getCurrentSlice();

dump($slice->getLink(1));
// -> 1

echo REX_LINK[id=1 output=url];
// -> ../index.php?article_id=1

Hier fehlt jeweils der Anker. Vermutlich handelst du das aber anders.

Und ich vermute, du befüllst auch die Artikelliste etwas anders. Dieses <li> manuell zu erzeugen, finde ich recht kompliziert. Aber ich bin gespannt wie du es nutzt.

[ynamite]

Den EP nutze ich mehr oder weniger genauso wie in deinem Beispiel oben:

if (rex::isBackend()) {
  rex_extension::register('LINKMAP_ARTICLE_LIST', function (rex_extension_point $ep) {
    $subject = $ep->getSubject();
    $subject.= '<li class="list-group-item"><a href="javascript:insertLink(\'redaxo://1#anker\',\'' . addslashes('Home') . '\u0020\u005B' . '1' . '\u005D #' . addslashes('anker') . '\');" class="rex-online"><i class="rex-icon fa fa-anchor"></i> ' . rex_escape('Anker') . '<span class="list-item-suffix">1</span></a></li>';
    $ep->setSubject($subject);
  });
}

In der Modul-Ausgabe parse ich dann REX_LINK[1] selbst (gibt bei mir dann 1#anker aus).
War mir gar nicht klar, dass es $slice->getLink(1) und output=url gibt :)

Da ich das nun kenne, kann ich gerne für beides eine Lösung liefern. Gibt es noch andere Stellen/Methoden/Ersetzungen, die es zu beachten gäbe?

[ynamite]

Methoden sind ergänzt.

$slice = $this->getCurrentSlice();

dump($slice->getLink(1));
// -> 1#anker

echo REX_LINK[id=1 output=url];
// -> ../index.php?article_id=1#anker

[tbaddade]

@ynamite Gregor und ich haben uns den PR noch einmal angeschaut und würden ihn aktuell erst einmal nicht übernehmen. Nur ein Beispiel: In der Datenbank werden derzeit nur IDs bzw. kommaseparierte IDs gespeichert – bei den nachgelagerten Checks und den verwendeten Regex würde das sonst zu Problemen führen.

Magst du deine Intention hinter dem PR und die Grundidee bitte noch einmal in einem Issue festhalten? Das Thema Anker ist uns bekannt und jeder von uns geht damit im Moment etwas anders um. Vielleicht ergibt sich später noch eine saubere Lösung, um Anker über die Linkmap auswählen zu können.

[ynamite]

@tbaddade Ich kann euren Entscheid verstehen, finde es aber etwas schade. FYI, ich habe das genauso in zwei Projekten im Einsatz und das funktioniert (soweit) problemlos. Mögt ihr genauer beschreiben, wo/wann das zu Problemen führen kann?

Klar, es ist unschön, dass ein ID Feld zweckentfremdet wird. Bei den Link und Linkmap Vars ist das aber eigentlich nicht so tragisch. Hier überwiegt der Nutzen, imo. Ohne signifikante Änderungen an der Tabelle der Slices sehe ich keine andere Möglichkeit, Ankerpunkte vernünftig zu realisieren. Aber ja, ich fände es natürlich super, wenn es eine bessere Lösung gäbe.

Mir stellt sich zudem gerade die Frage, weshalb die Link-Spalten ID-Spalten sein müssen. Wie wäre es mit JSON? Klar, breaking change und so, aber irgendwann muss man die Altlasten loswerden. Es ist tatsächlich so, dass es bis dato in Redaxo bzw. Struktur-Addon keine (standardisierte) Lösung gibt, um Ankerpunkte für Redakteure/Laien bereitzustellen. Bitte nicht als Seitenhieb verstehen, ich liebe Redaxo, aber das ist nach über 20 Jahren schon ein bisschen "naja".

Intention
Die Intention hinter dem PR war eine Lösung für das "Redaxo Ankerproblem" bereitzustellen, welche einfach zu integrieren ist und auf der bestehenden Linkmap und dem Linkfeld aufbaut, ohne dabei möglichst die herkömmliche Funktionalität einzuschränken und ohne tiefgreifende Änderungen auskommt.
Diese Lösung empfinde ich als eine benutzerfreundliche Variante, da die Funktionalität in den bestehenden Link-Dialogen integriert wird und somit aus UX-Sicht am richtigen Ort ist.

[gharlan]

Mögt ihr genauer beschreiben, wo/wann das zu Problemen führen kann?

Ich dachte, wir hätten einen isInUse-Check auch für Artikel, ähnlich wie wir ihn bei Medien haben (lassen sich nicht löschen, wenn noch irgendwo verwendet).
Aber hatte ich wohl falsch in Erinnerung. Wenn wir ihn hätten, dann würde der bei der Linklist vermutlich mit FIND_IN_SET oder REGEXP in der SQL-Query arbeiten, und da würde es dann neu nicht mehr funktionieren, wenn an der ID plötzlich ggf. noch ein Anker hängt.
Bzw. genauso bei einem =-Check bei den Einzel-Link-Feldern.

Somit sehe ich zurzeit aber doch keine existierende Stelle, wo es zu einem Problem führt, sondern nur in der Theorie, bei welcher Art von Abfragen es zu Problemen führen könnte (und evtl. auch führt, je nach Projekt-/Addon-Code).

Bin mir hier noch unsicher. So richtig gefallen will mir das nicht, auch wenn ich generell durchaus Sympathie für pragmatische Lösungen habe und bisher auch noch keine bessere Lösungsidee habe.

[ynamite]

Das Argument leuchtet ein. Wobei vermutlich eine Regexp möglich wäre, die allfällige Anker unterstützt.

Nur als Idee: vielleicht wäre ein zukunftsorientierter Ansatz, wenn alle REX_LINKs in einer eigenen Relations-Tabelle leben würden? Mit einer Relation zu SLICE_ID und entsprechenden Indexes. Keine Ahnung wie man das am besten löst. Oder vielleicht doch in rex_article_slice direkt als JSON-Spalte?

[gharlan]

Hinweis: Dieser Kommentar kommt von Claude (Anthropic, Opus 4.7). @gharlan hat mich um eine zweite Meinung zu diesem PR gebeten und postet meine Einschätzung hier unverändert. Die Entscheidung über den PR liegt natürlich bei den Maintainern.

Hi @ynamite, ich hab mir den Diff und die Diskussion durchgelesen. Das Anliegen ist absolut nachvollziehbar — Anker im Linkfeld auswählbar zu machen ist seit Jahren ein Schmerz, und der pragmatische Ansatz hat seinen Charme. Ein paar Punkte, die mir aus Review-Sicht aufgefallen sind:

Konkrete Probleme im aktuellen Diff

1. Wahrscheinlich ein Bug: zusätzliches ; am Ende des Outputs (var_link.php)

return 'rex_getUrl(' . self::quote($articleId) . ')' . ($hash ? ' . \'#\' . ' . self::quote($hash) : '') . ';';

Im Original war kein ; am Ende. Da der Rückgabewert vom VAR-System in größere Code-Kontexte eingebettet wird, kann das je nach Aufrufstelle zu doppelten Semikolons / Syntaxproblemen führen.

2. BC-Break in rex_article_slice::getLink()
Der Rückgabetyp wechselt von int|null auf string|null. Auf der 5.x-Branch ist das problematisch — Code wie if ($slice->getLink(1) === 5) oder strict-typed Parameter brechen still. Wenn das eingeführt wird, dann eher als zusätzliche Methode (getLinkRaw() o. ä.) und getLink() bleibt int.

3. Potenzieller XSS-Vektor
<input ... value="' . $value . '" /> und <option value="' . $link . '"> geben den Wert ungefiltert aus. Mit reiner numerischer ID war das safe; sobald die Spalte beliebige Strings enthalten kann (1#"><script>...), wird daraus eine Lücke. Müsste mit rex_escape($value, 'html_attr') abgesichert werden.

4. Off-topic Änderungen
Zwei Icon-Klassen werden umgedreht (rex-icon rex-icon-top → rex-icon-top rex-icon, dito bottom). Sieht nach versehentlichem IDE-Reformat aus — gehört nicht in diesen PR.

5. UX-seitig nur halb fertig
Der PR macht das Speichern/Auslesen von Ankern möglich, aber die eigentliche Redakteursfunktion (Anker in der Linkmap auswählen) muss jeder Anwender selbst per LINKMAP_ARTICLE_LIST-EP zusammenbauen. Out-of-the-box bringt der Core damit relativ wenig — und genau hier liegt ja eigentlich der Mehrwert für Endanwender.

Das größere Bedenken: Datenmodell

@tbaddade's Punkt mit der ID-Spalte halte ich für valide, auch wenn es heute im Core nirgends sichtbar weh tut.
link1-link10 sind semantisch Foreign Keys. Sobald da 1#anchor drin stehen kann, sind FIND_IN_SET-Lookups, JOINs gegen article.id und potenzielle künftige isInUse-Checks broken. Das ist nicht "der PR bricht Bestehendes", sondern "der PR versperrt Optionen, die wir uns offenhalten wollen".

Vorschlag für eine alternative Lösung

Statt den Anker in die ID-Spalte zu mischen, würde ich ihn separat halten:

Variante A — BC-safe für 5.x:
Zusätzliches VAR REX_LINK_ANCHOR[1] mit eigener Spalte (z. B. link1_anchor varchar(255)). Anker und ID bleiben sauber getrennt, kein BC-Break, SQL-Lookups auf der ID-Spalte funktionieren weiter. Die Linkmap-UI-Erweiterung (Anker-Auswahl im Dialog) lässt sich darauf aufbauend separat angehen.

Variante B — sauberer Schnitt für 6.x:
Link-Daten als JSON-Struktur oder eigene Relations-Tabelle, dann sind Anker, target=_blank, rel-Attribute etc. strukturell mitgedacht. Hattest du selbst ja auch schon angedeutet.

Variante A löst dein konkretes Problem ohne die genannten BC- und Datenmodell-Risiken. Wenn du Lust hättest, das in dieser Form noch mal anzugehen — ich denke, das hätte deutlich bessere Chancen auf einen Merge.

[gharlan]

Variante A löst dein konkretes Problem ohne die genannten BC- und Datenmodell-Risiken. Wenn du Lust hättest, das in dieser Form noch mal anzugehen — ich denke, das hätte deutlich bessere Chancen auf einen Merge.

Wobei ich kein Freund von Variante A bin. Also mag nicht dafür extra komplett neue Felder anlegen.

Ich denke wir müssen das Thema zurückstellen. Für R5 sehe ich da keine richtig zufriedenstellende Lösung.

[ynamite]

Fair und nachvollziehbar. Dann lassen wir das vorerst.

Wäre natürlich schon toll wenn eine passende Lösung beim nächsten Major-Release mit drin wäre.