Isotope und Contao 5

[Ernestopheles]

Bin dabei, zu analysieren, wo in Isotope Anpassungen für Contao 5 gemacht werden müssen.

Habe diese composer.json für Isotope hergenommen, wobei ich notificationcenter und conditionalselectmenu entfernt habe, da keine kompatiblen Versionen gefunden:

{    "name": "isotope/isotope-core",
    "description": "Isotope is the premier open source eCommerce solution for Contao Open Source CMS",
    "homepage": "https://isotopeecommerce.org",
    "keywords": [
        "contao",
        "isotope"
    ],
    "type": "contao-module",
    "license": "LGPL-3.0-or-later",
    "authors": [
        {
            "name": "terminal42 gmbh",
            "homepage": "https://www.terminal42.ch"
        }
    ],
    "funding": [
        {
            "type": "other",
            "url": "https://circle.isotopeecommerce.org"
        },
        {
            "type": "github",
            "url": "https://github.com/terminal42"
        },
        {
            "type": "other",
            "url": "https://ko-fi.com/terminal42"
        }
    ],
    "support": {
        "issues": "https://github.com/isotope/core/issues",
        "source": "https://github.com/isotope/core",
        "forum": "https://community.contao.org/de/forumdisplay.php?111-isotope",
        "docs": "https://docs.isotopeecommerce.org/"
    },
    "require": {
        "php": "^7.2 || ^8.0",
        "composer-runtime-api": "^2",
        "contao-community-alliance/composer-plugin": "^2.4 || ^3.0",
        "terminal42/dcawizard": "^2.3",
        "menatwork/contao-multicolumnwizard-bundle": "^3.4",
        "terminal42/contao-tablelookupwizard": "^3.2 || ^4.0",
        "terminal42/dc_multilingual": "^4.0",
        "tecnickcom/tcpdf": "^6.2.22",
        "patchwork/utf8": "^1.2",
        "webmozart/path-util": "^2.2",
        "symfony/filesystem": "^4.4 || ^5.0 || ^6.0",
        "symfony/http-client": "^4.4 || ^5.0 || ^6.0",
        "symfony/polyfill-php80": "^1.25"
    },
    "require-dev": {
        "terminal42/dcawizard": "^2.0",
        "codefog/contao-haste": "^5.1",
        "contao/core-bundle": "^5.2",
        "contao/manager-plugin": "^2.0",
        "terminal42/contao-fineuploader": "^2.0 || ^3.0",
        "terminal42/contao-changelanguage": "^3.0",
        "petschko/dhl-php-sdk": "dev-master@dev",
        "mpay24/mpay24-php": "^4.0",
        "friendsofphp/php-cs-fixer": "^2.14"
    },
    "suggest": {
        "terminal42/contao-fineuploader": "For file uploads in frontend using FineUploader",
        "petschko/dhl-php-sdk": "For DHL Business shipping",
        "mpay24/mpay24-php": "For mPAY24 payment provider",
        "terminal42/swissbilling-api": "For SwissBilling payment provider",
        "contao/tcpdf-bundle": "Required for the standard document type in Contao 4.7+"
    },
    "conflict": {
        "terminal42/contao-fineuploader": "< 2.0 || >= 4.0",
        "terminal42/contao-changelanguage": "< 3.0 || >= 4.0",
        "contao/manager-plugin": "<2.0 || >= 3.0",
        "mpay24/mpay24-php": "< 4.0 || >= 5.0"
    },
    "autoload": {
        "psr-0": {
            "Isotope\\": [
                "system/modules/isotope/library",
                "system/modules/isotope_reports/library",
                "system/modules/isotope_rules/library"
            ],
            "UnitedPrototype\\": [
                "system/modules/isotope/library"
            ]
        },
        "classmap": [
            "system/modules/isotope/drivers/"
        ]
    },
    "extra": {
        "contao-manager-plugin": "Isotope\\ContaoManager\\Plugin",
        "contao": {
            "sources": {
                "system/modules/isotope": "system/modules/isotope",
                "system/modules/isotope_reports": "system/modules/isotope_reports",
                "system/modules/isotope_rules": "system/modules/isotope_rules"
            },
            "runonce": [
                "system/modules/isotope/config/upgrade.php"
            ]
        }
    },
    "config": {
        "allow-plugins": {
            "contao-components/installer": true,
            "php-http/discovery": true,
            "contao/manager-plugin": true,
            "contao-community-alliance/composer-plugin": true
        }
    }
}

Somit lässt sich Isotope installieren. So weit erstmal gut.



Damit läuft dann jedoch beim composer update meines Contao 5 Projektes das contao-setup script auf Fehler, weil conditionalselectmenu nicht vorhanden.

Bin ich da auf dem richtigen Weg grundsätzlich?

Wenn ja, würde ich conditionalselectmenu forken und die Abhängigkeiten anpassen, evtl. dann auch mit dem notification center. Vielleicht lässt sich mein Testprojekt damit dann installieren?

[andreas.schempp]

Ich denke mit den Forks bist du nicht falsch. Zumindest um es zum laufen zu bekommen.

[Ernestopheles]

ok, das geht jetzt schrittweise voran, erstmal alle Fehler aus dem cache warmup zu beseitigen.

Frage: Wie ist das passende Vorgehen? Habe aktuell 2.8 ausgecheckt. Nun könnte ich einen neuen branch 3.0 anlegen und dort die Anpassungen für Contao 5.2 vornehmen. Später müssten wir dann alle späteren Änderungen auf 2.8 sowie 2.9 in die 3.0 mergen.

Oder sollte ich 2.9 auschecken und dann darauf den neuen branch 3.0 anlegen?

[andreas.schempp]

auf jeden Fall für 2.9. Und bitte für jede Kleinigkeit einen separaten Pull Request. Ich hab noch keine Vorstellung was du findest bzw. fixen wirst, also besser einzelne Dinge bevor du dir wochenlang Arbeit machst die ich dann nicht durchblicke ;-)

[Ernestopheles]

OK, dann mache ich das so.

Testen: Da könnte ich die Demo nehmen; Und dann alles mal durchspielen? Oder gibt es einen PHPUnit Test, den ich nur anschmeissen müsste? Und muss auch noch für PHP 7 und Contao 4.9 getestet werden (gemäß der composer.json ja, nur macht das auch Sinn?)???

[andreas.schempp]

Die Version 3 wäre aus meiner Sicht nur für Contao 5 und PHP 8. Aber wenn du Fehler findest die auch in der 2.9 für Contao 4 gefixt werden könnten/sollten, dann natürlich gerne dort. Tests gibt es leider keine.

[Ernestopheles]

Jetzt blick ich nicht mehr durch. Was ich vorhabe ist Isotope unter PHP 8 und Contao 5 zum Laufen zu bringen und entsprechende Änderungen einzubauen, per PR auzf den 2.9er branch wie gewünscht.

Meine Frage dazu war, wie und in welchem Umfang ich die PR testen kann/soll.
und ob Contao 5 PHP 8 hier reicht zu testen i
Was , wenn Anpassungen nicht rückwârtskompatibel wären?

[zoglo]

Was , wenn Anpassungen nicht rückwârtskompatibel wären?

Es gibt keine CONTRIBUTING.md, daher musst du auf die Antwort von Andy und den terminal42 Jungs warten.
Meines Erachtens ist das aber eine Entscheidung vom Maintainer des Projekts.

Ich versuche mal folgende Punkte aufzugreifen, diese auch zu erklären

Wenn Anpassungen nicht rückwärtskompatibel sind
Isotope E-Commerce gibt es schon seit glaube über einem Jahrzehnt, zusätzlich werden auch kostenpflichtige Erweiterungen dafür bereitgestellt (durch externe Dienstleister etc.).
Es ist eine der meistgeladenen Erweiterungen im Contao-Universum (https://packagist.org/packages/isotope/isotope-core) mit über 44 Tausend Downloads.
Jemand, der einen Shop betreibt, macht es aus finanziellen Gründen. Die meisten Kunden / Nutzer von Isotope wissen nicht einmal, dass Isotope eine freie Open-Source-Lösung ist.
Wenn nach einem "Update" etwas nicht funktioniert, flattern die E-Mails rein (Ob vom Kunden oder von einer Agentur).

Diese Installationen würden nicht mehr laufen, dementsprechend müsste man Migrationen programmieren, sofern möglich.

Alte Struktur
Wie bereits von Andy zu lesen, ist Isotope so alt, dass die Architektur stark veraltet ist.
Mit der neuen 3er Version möchte man also alles einmal umkrempeln, damit es in Zukunft einfacher ist.
Symfony gab es zu Zeiten von Isotope in Version 1 noch nicht, Version 2 ist in der Struktur auch stark veraltet, Symfony und neuere PHP-Versionen ermöglichen es, dass man viele *Altlasten* entfernen kann.


Mammutsprojekt
Es ist mehr Arbeit als du denkst. Wir reden hier von evtl. mindestens 150+ Stunden Arbeit für einen Senior-Entwickler (und weit darüber hinaus).
Kleine Pull-Requests helfen hierbei, dass man bei Reviews nicht mehr Zeit verbringt, als der Entwickler investiert hat.
Es wird undurchsichtig, schwer zu kontrollieren... und es könnte potenziell tausende Shops zerstören.

Daher langsam anfangen, contributen, mit Leidenschaft drangehen...
Oder jemanden finden, der so ein Mammutsprojekt finanziert, sodass die Jungs von terminal42 selber Hand anlegen können.


Fazit
Das ist alles nicht böse gemeint, ich möchte dir hier nur die Hand ans Herz legen und dich schützen, bevor du dich in etwas reinstürzt, was du nicht stemmen kannst.


Abhängigkeiten
Zudem ist das Notification-Center eine Abhängigkeit von Isotope, welche notwendig für die Bestellungen ist.
Bevor das Notification-Center nicht unter Contao 5.* funktioniert, würde ich sagen, dass es zu Problemen führen wird.

[Ernestopheles]

@zoglo: Danke für deine Ausführungen. ich glaube du verstehst mich miss. Ich habe mich bereit erklärt, bei den Anpassungen zu helfen, nicht das gesamte Projekt zu meistern. Wenn ich behilflich sein kann, würde ich gerne das machen, was ich auf meiner heutigen (beschränkten) Kenntnis / Einsicht in Symfony, Contao und Isotope erledigen kann, um die Sache voranzubringen.

Mein Vorgehen ist:
Auftretende Fehlermeldungen wie aktuell beim cache warmup analysieren und per PR beheben. Das mache ich unter einem Testprojekt unter PHP 8.1 und Contao 5.2.

Wenn ich PRs für 2.9 mache, dann müssen die, wenn ich das richtig sehe, für alle Contao Versionen ab 4.9 sowie alle PHP Versionen ab 7.2 getestet sein.

Unklar ist mir, WIE getestet werden sollte. Dafür wäre es nützlich, zu hören, wie andere das machen. Wenn es keine Testautomatisierung gibt, bleibt ja eigentlich nur der Online Test und dort alle Funktionen einzeln durchchecken, z.B. in der Isotope Demo, oder? Dafür ggf. Tools benutzen, die Bildschirmausgaben vergleichen oder Ähnliches? Wie könnte man einen automatisierten Test bauen? Habe bisher nur Erfahrung mit PHPUnit Tests für einzelne Klassen (speziell Listener).

Schlagt mir doch ein Vorgehen vor, das ihr für sinnvoll haltet.

Weitere Gedanken:
Klar laufen heute alle shops unter Contao < 5 und bestimmt auch vielfach unter PHP < 8 . Daher ist die Frage, ob es überhaupt Sinn macht, dort Updates zu machen, die für die neuen Versionen bestimmt sind. Daher mein ursprünglicher Gedanke, Anpassungen in einem neuen Branch 3.0 vorzunehmen. Aber vielleicht passt das ja nicht so gut.

Bevor das Notification-Center nicht unter Contao 5.* funktioniert, würde ich sagen, dass es zu Problemen führen wird.

Sehr wahrscheinlich! Es ist mir völlig klar, dass alle benötigten Extensions ebenfalls angepasst sein müssen, bevor Isotope unter 5 lauffähig ist. Wenn Isotope soweit ist, wird diese Voraussetzung vermutlich auch gegeben sein.

Wie gesagt: ich helfe gerne und tue was ich kann. Wir sollten uns auf eine Methodik einigen können und vielleicht gibt es ja Hinweise für das Testen.

[Ernestopheles]

Habe das gefunden:

https://symfony.com/doc/current/the-...-tests-crawlen

Damit könnte man wohl mit einigem Aufwand Tests schreiben, die z.B. die Isotope Demo durchspielen? Hat das vielleicht schon mal jemand gebaut und würde es zur Verfügung stellen?

[Ernestopheles]

Oder hat jemand ein cypress script für die isotope demo?

[andreas.schempp]

Ich kann hier @zoglo nur zustimmen, das wird sicher nicht einfach. Aber wenn du ausprobieren möchtest, natürlich gerne.

Bezüglich Branch: solange die 2.9 nicht veröffentlicht ist, mache ich noch keinen 3.0 Branch. Aber du kannst deinen PRs einfach gegen 2.9 stellen, und zur gegebenen Zeit machen wir ein Rebase für 3.0. Contao 4 und PHP 7 brauchst du nicht testen.

Bezüglich Notification Center: die 2.0 steht in den Startlöchern (siehe https://github.com/terminal42/contao...iscussions/277), mit Isotope 3 würde sicher nur noch diese Version supported. Alles was mit den Notifications zu tun hat können wir erst anpassen sobald Notification Center 2 veröffentlicht wird (und da wird eigentlich alles geändert werden müssen ;-)).

[Ernestopheles]

Super, dann machen wir das so. Werde mir Cypress anschauen und vermutlich damit testen.

[Ernestopheles]

Moin,

mache mit gerade mit cypress vetraut. In der Isotope Demo (lokal gehostet) bekomme ich beim Loginversuch diese Fehlerseite:

Ungültiges Anfrage-Token

Was ist das Problem?

Das Request-Token konnte nicht validiert werden.
Wie kann ich das Problem lösen?

Bitte klicken Sie hier und versuchen Sie es erneut. Verwenden Sie nicht die Zurück-Schaltfläche Ihres Browsers.
Weitere Informationen

Der Fehler tritt bei einer POST-Anfrage ohne gültiges Request-Token auf. In Contao 2.10 wurde die Referer-Prüfung durch ein Request-Token-System ersetzt. Wenn das Problem anhält, verwenden Sie eventuell eine inkompatible Drittanbieter-Erweiterung oder haben Ihre Contao-Installation nicht vollständig aktualisiert.


Besuchen Sie die Contao-Supportseite für weitere Informationen.

Laut Entwicklerhandbuch wird aber bei nicht authentifizierten Anwendern keine CSRF Prüfung gemacht:

Providing CSRF protection for users that are not authenticated against the app does not make any sense. So if you visit a regular Contao page with a form placed on it, you will not necessarily see any cookies being set in your browser. Only if you are authenticated in such a way that the browser will automatically send authentication information along without any user interaction (e.g. any cookies or basic authentication via Authorization headers), CSRF protection is required. So don’t get fooled by the cookies not being present all the time, Contao is actually very smart about them to improve HTTP cache hits.

https://docs.contao.org/dev/framewor...ns/#body-inner

Wenn die isotope demo seite ohne cypress läuft, gelingt der login.

Wo ist da jetzt das Problem?

Habe auch versucht, ein CSRF Token im header des responses aus dem cypress request zu finden, da ist keines (was ja laut Entwicklerhandbuch auch stimmig ist). Warum dann dennoch die Fehlermeldung kommt, erschliesst sich mir nicht.

Die Demo läuft unter contao 4.13 und PHP 8.1

[Ernestopheles]

In der .env.local setze:
COOKIE_ALLOW_LIST=PHPSESSID,csrf_https-contao_csrf_token,csrf_contao_csrf_token,trusted_d evice,REMEMBERME

https://docs.contao.org/dev/referenc...kie-allow-list

Das verhindert den CSRF Token Fehler in der prod-Umgebung. Für APP_ENV=dev habe ich leider noch keine Lösung gefunden.

Es gibt einen slack thread dazu, der mich auf die allow_list gebracht hat https://contao.slack.com/archives/CK...61810787788719 , so ganz schlau werde ich aus dem Ganzen noch nicht.

[Ernestopheles]

https://github.com/Ernestopheles/isotopeDemo

Bin ich da auf dem richtigen Weg?
1. Ich schreibe Tests für die demo, um meine Anpassung am isotope Core (bezüglich contao 5) zu testen.

2. Vielleicht können die Tests ja auch für andere nützlich sein? Deshalb pushe ich sie in das demo rep. Würde das dann, vorausgesetzt PR und gemerged, eine neue Version der .zip geben? Und wir könnten in die Readme eine Installationsanweisung für cypress einfügen?

[Ernestopheles]

https://github.com/Ernestopheles/isotopeDemo
Die sollten eigentlich ausreichen, oder?

Welche backend tests sind vonnöten für meine PR?

[Ernestopheles]

So, nun geht's weiter mit der eigentlichen Entwicklung. Ist es richtig, ecs und phpstan einzusetzen?

[andreas.schempp]

Ich kann dir deine Fragen leider nicht wirklich beantworten. Wie du entwickelst (ecs, phpstan) ist dir überlassen. Allerdings sind das Code Quality Tools, und ich selber würde die nicht für erste Tests nutzen. Ich würde die Tests von Hand machen. Aber da ich mich zur Zeit nicht mit der Materie beschäftige, kann ich dir nicht sagen was du machen sollst. Ich würde vermutlich erst mal sehen welche Fehler bei der Installation und Verwendung kommen, und versuchen dafür Korrekturen (Pull Requests) vorzunehmen. Kein Code Qualtity Tool wird dir sagen was du refactorn musst.

[Ernestopheles]

Sorry, habe vielleicht zu wenig erläutert.

Entwicklung und Test have ich bereits in Arbeit und hier beschrieben wie ich vorgehen.

Es geht mir um die Formatierung in erster Linie, da ist ja standardmäßig ecs im Einsatz soweit ich sehe.
Darauf zielt meine Frage, ob die Umformatierungen die ECS macht auch eurem Standard entsprechen. Es wäre ja blöd, wenn meine PR unzählige Änderungen aufweisen, die nur durch andere Formatierung entstanden sind...

Und bei Phpstan ist mir jetzt unklar ob das noch eingesetzt werden soll, weil es ja kürzlich erst als deprecated getaggt wurde. Leider steht nicht dabei was anstelle dessen genommen werden sollte.

[andreas.schempp]

ah, verstehe. Bitte die Formattierung von bestehendem Code nicht ändern, sonst haben wir extrem viel "tote" Änderungen ohne Mehrwert. Ich würde das dann ggf. selber in einem Schritt machen, das ist ja kein Aufwand und geht ja automatisch. Also wenn du Dinge findest und Pull Requests machst, immer nur die nötigen Zeilen ändern.

PS: wir orientieren und generell am "modernen" Code-Style von Contao (nicht am alten mit den Tabs), also primär PSR-2 etc.

[Ernestopheles]

also primär PSR-2 etc.

Du meinst vermutlich PSR-12?

[zoglo]

Du meinst vermutlich PSR-12?

Ich denke, dass hier wirklich PSR-2 gemeint war

[Ernestopheles]

Wo kann ich nachsehen, welche Standards Contao verwendet?

Psr-2 ist deprecated, ebenso contao-phpstan...

[zoglo]

Psr-2 ist deprecated, ebenso contao-phpstan...

Dann sei der Vorreiter und setze auf PSR-4 wie die meisten der "neuen/gepflegten" (Contao-Manager-kompatiblen-)Erweiterungen für Contao ^5.0

(Isotope hatte leider noch nicht die Manneskraft / die Finanzierung für ein richtiges Update auf Contao 4.9+ / 5.x)

[Ernestopheles]

Sorry ich verstehe dich leider nicht, vielleicht fehlen mir die Hintergründe dazu.

Was ich finde ist dies
[Deprecated - As of 2019-08-10 PSR-2 has been marked as deprecated. PSR-12 is now recommended as an alternative.]

[Ernestopheles]

Konkret habe ich lange gesucht, warum meine lokale Version des isotope-core in meiner Demo-Webseite nicht herangezogen wurde:

Normalerweise denkt mensch ja, dass ein composer.json Eintrag wie hier genügt:

{    "type": "project",
    "repositories": [
        {
            "type": "path",
            "url": "../isotope/isotopeDemo"
        }
    ],
    "require": {
        "contao/calendar-bundle": "4.13.*",
        "contao/comments-bundle": "4.13.*",
        "contao/conflicts": "*@dev",
        "contao/faq-bundle": "4.13.*",
        "contao/listing-bundle": "4.13.*",
        "contao/manager-bundle": "4.13.*",
        "contao/news-bundle": "4.13.*",
        "contao/newsletter-bundle": "4.13.*",
        "isotope/isotope-demo": "@dev"
    },
    "extra": {
        "public-dir": "public",
        "contao-component-dir": "assets"
    },
    "scripts": {
        "post-install-cmd": [
            "@php vendor/bin/contao-setup"
        ],
        "post-update-cmd": [
            "@php vendor/bin/contao-setup"
        ]
    },
    "config": {
        "allow-plugins": {
            "contao-components/installer": true,
            "php-http/discovery": true,
            "contao-community-alliance/composer-plugin": true,
            "contao/manager-plugin": true,
            "isotope/isotope-demo": true
        }
    }
}

Denn damit sage ich Composer, dass für meine Demo-Webseite die lokale Entwicklungsversion von Isotope-demo genommen werden soll.

Diese wiederum hat diese composer.json:

{    "name": "isotope/isotope-demo",
    "description": "Demo Version of Isotope eCommerce",
    "homepage": "https://isotopeecommerce.org/en/demo.html",
    "keywords":["contao", "isotope"],
    "type": "composer-plugin",
    "license": "LGPL-3.0-or-later",
    "version": "9",
    "authors":[
        {
            "name": "Isotope Workgroup",
            "homepage": "https://isotopeecommerce.org/en/team.html"
        }
    ],
    "support": {
        "issues": "https://github.com/isotope/demo/issues",
        "source": "https://github.com/isotope/demo"
    },
    "repositories": [
        {
            "type": "path",
            "url": "../isotopeCore"
        }
    ],
    "require": {
        "php": "^7.4 || ^8.0",
        "isotope/isotope-core": "@dev",
        "composer-plugin-api": "^1.1 || ^2.0",
        "contao/manager-bundle": "4.13.*",
        "terminal42/contao-easy_themes": "^3.0",
        "terminal42/contao-changelanguage": "^3.1"
    },
    "require-dev": {
        "contao/manager-plugin": "^2.7",
        "composer/composer": "^1.9"
    },
    "conflict": {
        "contao/manager-plugin": "<2.0 || >= 3.0"
    },
    "autoload": {
        "psr-4": {
            "Isotope\\Demo\\": "src/"
        }
    },
    "extra": {
        "class": "Isotope\\Demo\\Composer\\Plugin",
        "contao-manager-plugin": "Isotope\\Demo\\ContaoManager\\Plugin"
    },
    "minimum-stability": "dev",
    "prefer-stable": true,
    "config": {
        "allow-plugins": {
            "contao-components/installer": true,
            "php-http/discovery": true,
            "contao/manager-plugin": true,
            "contao-community-alliance/composer-plugin": true,
            "dealerdirect/phpcodesniffer-composer-installer": true
        }
    }
}

und verwendet wiederum selber eine lokale Entwicklungsversion des isotope-core. (Die Versionsnummer habe ich nur zu Prüfzwecken geändert.)

Nun - so habe ich gedacht, sei alles paletti, aber Pustekuchen: Composer überträgt die Anweisung, isotope-core aus dem path zu nehmen, zwar korrekt in die composer.lock aber nur in die von isotope-demo. Die composer.lock meiner Demoseite hat einen solchen Vermerk nicht, mit der Folge, dass meine Demoseite mit der Entwicklungsversion gemäß packagist läuft.

Merke: Alle Abhängigkeiten mit lokaler Ablage müssen - bis auf die unterste Abhängigkeits-Ebene - "oben" vermerkt werden!

[Ernestopheles]

Wie soll mit den - unter Contao5 fehlenden - Haste Klassen

 Haste\Units\...

verfahren werden?

Bleibt die Applikationslogik von Isotope? Dann wären die Haste Klassen "einfach" durch andere zu ersetzen, wie @Codefog schreibt:

• Haste\Units – use an alternative, for example jordanbrauer/unit-converter.

Oder ändert sich hier mit Contao5 mehr in Isotope?

[Ernestopheles]

Please take a look at my PRs

Is it convenient for you to have several PR (for adaption to Contao 5) for the same file? I could also cherry-pick them to only 1 PR.

Also config.php is 2 or 3 times present in the folder structure and presumably the changes will be equal to each file. So maybe 1 PR for all config.php stuff?

[Spooky]

Der erste PR betrifft das Vorkommen von array_insert: https://github.com/isotope/core/pull/2489
Du solltest eher alle Vorkommen in einem PR ändern. Du hast außerdem vergessen die Requirements in der composer.json anzupassen. Im 2.9 Branch steht immer noch

"contao/core-bundle": "^4.9"

ArrayUtil gibt es aber in 4.9 noch nicht.

Dein dritter PR betrifft das Vorkommen von TL_MODE: https://github.com/isotope/core/pull/2491
Auch da würde ich eher alle Vorkommnisse in einem PR ändern - also nicht ein PR pro Datei.

[Ernestopheles]

Du solltest eher alle Vorkommen in einem PR ändern.

Absolut! ich ändere das.

ArrayUtil gibt es aber in 4.9 noch nicht.

@andreas.schempp: Was denkst du, wie passt es am besten in deine Vorgehensweise? Alternativ könnte ich

array_splice

verwenden.

[Ernestopheles]

Es sind zahlreiche Dateien in Isotope betroffen, wo überall TL_MODE ersetzt werden muss.

Daher meine Idee, die Prüfung auszulagern:

// Scope.php

namespace Isotope\Helper; 


use Contao\System;
use Symfony\Component\HttpFoundation\Request;


class Scope
{
    public static function isBackend(): bool
    {
        $container = System::getContainer();
        $scopeMatcher = $container->get('contao.routing.scope_matcher');
        $requestStack = $container->get('request_stack');


        return $scopeMatcher->isBackendRequest($requestStack->getCurrentRequest() ?? Request::create(''));
    }


    public static function isFrontend(): bool
    {
        $container = System::getContainer();
        $scopeMatcher = $container->get('contao.routing.scope_matcher');
        $requestStack = $container->get('request_stack');


        return $scopeMatcher->isFrontRequest($requestStack->getCurrentRequest() ?? Request::create(''));
    }
} 

und dann in den Dateien

if ('BE' === TL_MODE) 

refactor gegen

if (Scope::isBackend()) 

und ein

use Isotope\Helper\Scope; 

oben einfügen



analog für Frontend




Vielleicht sogar diese Helperklasse in den Contao Core übernehmen?

Nur eine Idee, vielleicht unpassend...

[Ernestopheles]

Ich hoffe, dass ich damit die besprochenen Änderungen richtig umgesetzt habe. Wenn nicht, ok, ich lerne gerne dazu.

https://github.com/isotope/core/pull/2490

https://github.com/isotope/core/pull/2494

https://github.com/isotope/core/pull/2495

P.S. Die Änderung der Abhängigkeiten in der composer.json habe ich noch nicht eingebaut (betrifft PR 005), weil ich das gerne noch mit euch besprechen möchte.

[Spooky]

Noch zwei Anmerkungen:

• Du solltest deine PR entsprechend benennen. Statt "C4 004 Adaption towards Contao5" sollte der Titel beschreiben, was genau der PR macht (bspw. "Get rid of TL_MODE constant usage"). Auch die Beschreibung des PRs sollte näher erläutern, was der PR macht.
• Du musst nicht bestehende PRs schließen und neue aufmachen, wenn du etwas änderst. Ändere einfach den bestehenden PR.

[Spooky]

P.S. Die Änderung der Abhängigkeiten in der composer.json habe ich noch nicht eingebaut (betrifft PR 005), weil ich das gerne noch mit euch besprechen möchte.

Wenn du ArrayUtil nutzen möchtest, musst du diese Änderung in der composer.json machen.

[Ernestopheles]

Ich überlege, wie ich am sinnigsten weiter mache.

Nun kommen als Nächstes die Fehler wegen der in Haste 5 enternten Unit Klassen. Dazu hatte ich hier schon was geschrieben.

Parallel dazu habe ich mal ecs und phpstan (phpstan\phpstan) laufen lassen. ecs kann in 33 Dateien Use-Statements fixen. Normalerweise würde ich sowas immer zuerst laufen lassen, damit diese Fehler schon mal weg sind. Ich werde dafür einen separaten PR aufmachen.

Und PHPStan liefert 2277 errors... Ich sehe da z.B. die Fehler wegen der Haste Geschichten und z.B. Contao\Session, was alles vermutlich Contao5 / Haste 5 geschuldet ist. Aber genau darum geht es ja hier.

Haste\Unit wird in 8 Dateien benutzt. Ich gehe mal davon aus, dass die Anwendungslogik unverändert bleibt. Dann analysiere ich jetzt wofür es verwendet wird und schaue wie ich es ersetzen kann.

[andreas.schempp]

Sobald wir (dank deinem Pull Request) die Mindestanforderung auf Contao 4.13 heben, gibt es vermutlich einige Dinge die wir anpassen können und müssen. Es gibt auch schon diverse Pull Requests von anderen die darauf warten. Bisher habe ich mit der Anforderung an 4.13 gezögert, aber deine Arbeit zeigt dass wir da eigentlich nicht drum herum kommen. Danke!

[Ernestopheles]

Habe lokal ecs, phpstan und rector analog zum Contao Core eingebaut. Soll ich das schon als PR anbieten oder wollt ihr lieber damit warten?

[andreas.schempp]

für die Fehlerfindung ist das sicher gut, aber bitte (noch) nicht in den Core.

[Ernestopheles]

Die Isotope Demo z.B. verwendet - da mehrsprachig - in der config.yaml ein

prepend_locale: true 

.

Solange Isotope die Hooks

getRootPageFromUrl 

und

getPageIdfromUrl 

verwendet, kann das legacy_routing nicht deaktiviert werden.

Unter Contao 4 ist das ok; unter Contao 5 führt die Option aber zu einem Fehler:

In ArrayNode.php line 327:
Unrecognized option "prepend_locale" under "contao". Available options are "backend", "backup", "console_path", "crawl", "cron", "csrf_cookie_prefix", "csrf_token_name", "editable_files", "error_level", "image"
, "insert_tags", "intl", "localconfig", "locales", "mailer", "messenger", "pretty_error_screens", "preview_script", "sanitizer", "search", "security", "upload_path", "web_dir".

Das ist - soweit ich sehen konnte - im Contao Handbuch noch nicht dokumentiert.

Wenn ich das alles so richtig sehe, dann müssen wir also die beiden Hooks ersetzen!

Übrigens wird auch die Option

legacy_routing: true/false 

unter Contao5 nicht mehr unterstützt.