Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement YOrm dynamic return type extension #408

Merged
merged 24 commits into from
Mar 19, 2023
Merged

implement YOrm dynamic return type extension #408

merged 24 commits into from
Mar 19, 2023

Conversation

staabm
Copy link
Member

@staabm staabm commented Mar 18, 2023

closes #407

@staabm
Copy link
Member Author

staabm commented Mar 18, 2023

related phpstan upstream issue: phpstan/phpstan#9055
(regarding current false positives)

@staabm
Copy link
Member Author

staabm commented Mar 18, 2023

ich denke es könnte so funktionieren.. hab aber aktuell kein setup um es zu testen

@staabm staabm marked this pull request as ready for review March 18, 2023 09:27
Copy link
Member

@gharlan gharlan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sehr cool 👍. Soweit ich das einschätzen kann, sieht der Code richtig aus, bis auf die eine Sache.
Ich teste es aber gleich auch noch.

}
$modelClass = rex_yform_manager_dataset::getModelClass($relation['table']);
if ($modelClass === null) {
throw new \RuntimeException('Unable to map table to model: '.$relation['table']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wir setzen konsequent für jede Table eine eigene Model-Class bei yakamara.
Aber man muss das nicht tun. Man kann auch nur mit der Basis-Klasse arbeiten, oder auch teils teils.

Daher sollte denke ich keine Exception kommen, wenn es keine Model-Class für die Tabelle gibt.
Es bleibt dann halt bei der Basis-Dataset-Klasse, wie im yform-code als return-type annotiert.

@gharlan
Copy link
Member

gharlan commented Mar 18, 2023

Das läuft über die phpstan-dba-Extension, richtig?

Wenn ich die aktiviere, verschwindet der Fehler.
Allerdings bleibt er verschwunden, auch wenn ich die Model-Class gar nicht registriere, oder auch wenn die Methode eine völlig falsche Class als Return-Wert hat.

class Product extends rex_yform_manager_dataset
{
    public function getCategory(): ProductCategory // auch wenn hier was anderes, falsches steht, kommt trotzdem kein Fehler mehr
    {
        return $this->getRelatedDataset('category_id');
    }
}

class ProductCategory extends rex_yform_manager_dataset
{
}

rex_yform_manager_dataset::setModelClass('rex_data_product', Product::class);

// auch wenn diese Zeile fehlt, kommt trotzdem kein Fehler mehr
rex_yform_manager_dataset::setModelClass('rex_data_product_category', ProductCategory::class);

@staabm
Copy link
Member Author

staabm commented Mar 18, 2023

Das läuft über die phpstan-dba-Extension, richtig?

Ja

auch wenn ich die Model-Class gar nicht registriere

Wo passieren diese Registrierungen? Bei addon boot? Oder sind die irgendwo im code ?


Du kannst mit \PHPStan\dumpType() schauen welche werte phpstan an den stellen ermittelt, z.B

class Product extends rex_yform_manager_dataset
{
    public function getCategory(): ProductCategory // auch wenn hier was anderes, falsches steht, kommt trotzdem kein Fehler mehr
    {
        $x = $this->getRelatedDataset('category_id');
        \PHPStan\dumpType($x);
        return $x;
    }
}

@gharlan
Copy link
Member

gharlan commented Mar 18, 2023

Wo passieren diese Registrierungen? Bei addon boot? Oder sind die irgendwo im code ?

Meist in project boot.php oder im theme ordner bei theme-nutzern.

Du kannst mit \PHPStan\dumpType() schauen welche werte phpstan an den stellen ermittelt, z.B

Es kommt NEVER raus:

Bildschirm­foto 2023-03-18 um 13 23 33

@staabm
Copy link
Member Author

staabm commented Mar 18, 2023

kannst du mir kurz zusammenschreiben wie ich yform bei mir lokal in einer nackten redaxo version einstellen muss damit ich so einen fall reproduzieren kann?

(ich hab es noch nie verwendet)

@gharlan
Copy link
Member

gharlan commented Mar 18, 2023

In YForm gehe auf "Tableset importieren" und importiere dieses Set: tableset.json.zip

Und dann diesen Code verwenden:

class Product extends rex_yform_manager_dataset
{
    public function getCategory(): ProductCategory
    {
        $x = $this->getRelatedDataset('category_id');
        \PHPStan\dumpType($x);
        return $x;
    }
}

class ProductCategory extends rex_yform_manager_dataset
{
    /** @return rex_yform_manager_collection<Product> */
    public function getProducts(): rex_yform_manager_collection
    {
        $x = $this->getRelatedCollection('products');
        \PHPStan\dumpType($x);
        return $x;
    }
}

rex_yform_manager_dataset::setModelClass('rex_data_product', Product::class);
rex_yform_manager_dataset::setModelClass('rex_data_product_category', ProductCategory::class);

@gharlan
Copy link
Member

gharlan commented Mar 18, 2023

Frage ist noch, wie es bei getRelatedDataset bzgl null sein sollte.
Man stellt in yform bei der Relation ein, ob die leer sein darf oder nicht.

In dem Tableset oben habe ich die Kategorie im Produkt als Pflicht gesetzt.
Entsprechend habe ich als Return-Type dann auch : ProductCategory genutzt, also nicht nullable.

Gleichzeitig ist es aber so, dass yform zwar beim Erstellen/Bearbeiten von Produkten die Kategorie zur Pflicht macht, aber nicht an anderen Stellen drauf achtet. Zum Beispiel, wenn man eine Kategorie löscht, dann gibt es doch plötzlich Produkte ohne Kategorie.

Also entweder sagen wir, dass es nun mal eigentlich Pflicht ist, und entsprechend löst rexstan als nicht-nullable auf.
Oder rexstan ist ehrlich, dass es unter Umständen eben doch null sein kann, weil yform das insgesamt noch nicht so schön gelöst hat bzgl Pflicht-Relationen.
Letzteres bedeutet, dass man dann eben doch selbst noch eine Assertion setzen müsste, falls man bei sich einen non-nullable-Return will.

    public function getCategory(): ProductCategory
    {
        return rex_type::notNull($this->getRelatedDataset('category_id'));
    }

@staabm
Copy link
Member Author

staabm commented Mar 18, 2023

In YForm gehe auf "Tableset importieren" und importiere dieses Set: tableset.json.zip

grafik

:)

muss ich das ZIP entpacken vorher?

@gharlan
Copy link
Member

gharlan commented Mar 18, 2023

muss ich das ZIP entpacken vorher?

Ja. Github erlaubt keine JSON-Dateiuploads.

@gharlan
Copy link
Member

gharlan commented Mar 18, 2023

Wobei ich das nicht kapiere. Denn die Klassen müssten doch gar nicht vom Autoloader geladen werden, da sie schon beim Booten verfügbar sind, oder nicht?

@staabm
Copy link
Member Author

staabm commented Mar 18, 2023

Wobei ich das nicht kapiere. Denn die Klassen müssten doch gar nicht vom Autoloader geladen werden, da sie schon beim Booten verfügbar sind, oder nicht?

phpstan analysiert code statisch ohne ihn auszuführen. er findet klassen via autoloading zur analyse zeit
(manche meiner rexstan extensions brechen mit diesem prinzip weil ich zum teil doch code zur analyse-zeit ausführe.. aber das ist nur ein sehr kleiner teil)

@gharlan
Copy link
Member

gharlan commented Mar 18, 2023

Aber wie kann dann hier in deinem Extension-Code rex_yform_manager_dataset::getModelClass funktionieren?
Das Gegenstück (setModelClass) wird doch in der gleichen boot.php in meinem Fall gerade aufgerufen?

@staabm
Copy link
Member Author

staabm commented Mar 18, 2023

in deinem code beispiel aus #408 (comment) sind die setModelClass aufrufe drinn. und den code aus dem kommentar hab ich 1:1 required.

@staabm
Copy link
Member Author

staabm commented Mar 18, 2023

in einem echten redaxo projekt würde es nur funktionieren wenn die setModelClass im boot-path sind.
für $this->getRelatedDataset('category_id'); aufrufe deren model-class nicht im bootpath definiert wird, würde die extension die return types nicht präzisieren können und die false positives würden in diesem fall dann erhalten bleiben (urspr. issue)

@gharlan
Copy link
Member

gharlan commented Mar 18, 2023

in einem echten redaxo projekt würde es nur funktionieren wenn die setModelClass im boot-path sind.

Aber ich habe es bei mir doch im boot-path, in der boot.php vom project-Addon. So haben wir das ja auch bei unseren realen Projekten (siehe redaxo.org-Repo). Oder was meinst du mit boot-path?
Bloß dass die Klassen dort auch notiert sind, ist unrealistisch. Daher habe ich die bei mir jetzt mal ausgelagert.

@staabm
Copy link
Member Author

staabm commented Mar 18, 2023

Aber ich habe es bei mir doch im boot-path, in der boot.php vom project-Addon. So haben wir das ja auch bei unseren realen Projekten (siehe redaxo.org-Repo). Oder was meinst du mit boot-path?
Bloß dass die Klassen dort auch notiert sind, ist unrealistisch. Daher habe ich die bei mir jetzt mal ausgelagert.

wollte dir nur den unterschied erklären zwischen dem was ich getestet habe und dem wie es in einem echten projekt ist.

ich denke die extension sollte funktionieren mit setModelClass(...) im boot-path und den dazugehörigen klassen im normalen lib (autoloading) - und das müsste der realität entsprechen (und auch das was wir ohnehin für redaxo auch brauchen unabh. von rexstan)

ich denke wir reden vom gleichen und der PR funktioniert somit wie gewünscht?

}

// @phpstan-ignore-next-line
$datasetObject = call_user_func([$classReflection->getName(), 'create']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aus dieser Zeile bekomme ich nun diesen Fehler aus yform:

Missing $table declaration for model class "Product"

     #0 /Users/gharlan/workspace/redaxo/redaxo/src/addons/yform/plugins/manager/lib/yform/manager/dataset.php(51):
     rex_yform_manager_dataset::modelToTable()
     #1 [internal function]: rex_yform_manager_dataset::create()
     #2
     /Users/gharlan/workspace/redaxo/redaxo/src/addons/rexstan/lib/extension/YOrmDatasetRelatedDataDynamicReturnTypeExtension.php(74):
     call_user_func(Array)

Meine Model-Class-Registrierung aus der project-boot.php scheint also nicht zu greifen.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

der grund dass die boot.php der addons nicht eingebunden&berücksichtigt wird ist, dass hier im bootstrapping das includen der packages nicht aktiv ist:

$REX['LOAD_PAGE'] = false;

@staabm
Copy link
Member Author

staabm commented Mar 18, 2023

über

bootstrapFiles:
- phpstan-bootstrap.php

wird der core und aktive addons vor der analyse gebooted,.. es könnte sein dass phpstan nur statische symbole wie defines von konstanten oder deklarationen von funktionen/klassen wahrnimmt, aber keinen code der ausgeführt wird... hmm

@staabm
Copy link
Member Author

staabm commented Mar 18, 2023

aber ich hab noch eine idee

@christophboecker
Copy link
Member

Wo passieren diese Registrierungen? Bei addon boot? Oder sind die irgendwo im code ?

Meist in project boot.php oder im theme ordner bei theme-nutzern.

Könnte auch gezielt an anderer Stelle passieren.

Beispiel: Der Datensatz hat 20 YForm-Felder, von denen aber in einem bestimmten Kontext (inline-Formular via Feldtyp be_manager_relation ) nur ein Felder-Subset zulässig sein soll. Das kann man lösen, indem es für die Tabelle einen normalen Dataset gibt (class MySubTable extends rex_yform_manager_dataset) und einen weiteren mit reduziertem Formular (class MySubTableInline extends MySubTable). Letztere wird nur temporär herangezogen während der Formularausführung der Haupttabelle und dann wieder zurückgesetzt.

@staabm
Copy link
Member Author

staabm commented Mar 18, 2023

Letztere wird nur temporär herangezogen während der Formularausführung der Haupttabelle und dann wieder zurückgesetzt.

@christophboecker Danke für das Beispiel. Ich denke nicht dass solche beispiele mit rexstan unterstützbar sein werden.

Aber müsste man am konkreten codebeispiel sehen (bitte in neuem issue nicht hier)

@staabm
Copy link
Member Author

staabm commented Mar 19, 2023

@gharlan bitte nochmal testen. mit dem neusten change funktioniert es bei mir jetzt lokal (ohne manuelle hacks / setModelClass in der boot.php eines addons vorrausgesetzt):

grafik

@gharlan
Copy link
Member

gharlan commented Mar 19, 2023

Jupp der Fall funktioniert nun. 👍


Problematisch ist nun noch, wenn man nur teilweise Model-Classes verwendet, oder gar keine. Hier kommt aus beiden dumps aktuell *NEVER* raus, erwarten würde ich aber beim oberen rex_yform_manager_dataset, beim unteren rex_yform_manager_collection<Product>:

class Product extends rex_yform_manager_dataset
{
    public function getCategory(): rex_yform_manager_dataset
    {
        $x = $this->getRelatedDataset('category_id');
        \PHPStan\dumpType($x);
        return $x;
    }
}

rex_yform_manager_dataset::setModelClass('rex_data_product', Product::class);

function foo(): void {
    $category = rex_yform_manager_dataset::require(1, 'rex_data_product_category');
    \PHPStan\dumpType($category->getRelatedCollection('products'));
}

Und dann ist noch die Frage, ob der Type nullable sein sollte bei getRelatedDataset: #408 (comment)

@staabm
Copy link
Member Author

staabm commented Mar 19, 2023

Problematisch ist nun noch, wenn man nur teilweise Model-Classes verwendet, oder gar keine.

hab die cases nicht 100% verstanden, aber ich vermute der fehlende fix ist committed


Und dann ist noch die Frage, ob der Type nullable sein sollte bei getRelatedDataset: #408 (comment)

ich denke wir sollten null drinnen lassen, solange yform technisch nicht selbst sicherstellt dass es nicht mehr drinnen sein kann - hab ich jetzt so mal umgesetzt.

@gharlan
Copy link
Member

gharlan commented Mar 19, 2023

hab die cases nicht 100% verstanden, aber ich vermute der fehlende fix ist committed

Ist nun besser, und können wir so lassen, denke ich. 👍

Beim unteren Dump bei meinem Beispielcode kommt nur die allgemeine rex_yform_manager_collection raus bei rexstan, besser wäre rex_yform_manager_collection<Product>. Allerdings wäre die Umsetzung dafür etwas komplizierter für rexstan.
Und da ich sowieso empfehle, für alle Tabellen Model-Classes anzulegen, und da der Type nicht falsch ist, nur nicht so genau, würde ich es so lassen, wie du es jetzt hast.

Copy link
Member

@gharlan gharlan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Danke!

@staabm staabm merged commit 6c63671 into main Mar 19, 2023
@staabm staabm deleted the yorm branch March 19, 2023 09:27
@staabm
Copy link
Member Author

staabm commented Mar 19, 2023

Released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

YOrm: Dynamische Return-Types für getRelatedDataset, getRelatedCollection und getRelatedQuery
3 participants