Entwicklung eines Sichtbarkeits-Filterungs-Systems für wf-Anwendungen #1
Conversation
Danke, composer-require-checker! <3
There was a problem hiding this comment.
Hi Jano,
die Symfony- und Doctrine-Innereien kann ich nicht beurteilen. Mir fehlt wie oben gesagt in der Readme ein Anwendungsbeispiel, und ich vermute, dass es irgendwo auch noch ein Projekt oder einen Branch gibt, wo man dein Bundle im Einsatz sehen kann. Dazu würde ich mich noch über einen PR freuen.
Außerdem fände ich es gut, wenn du einige Basis-Checks noch einrichtest, z. B. den cs-style-fixer. Auch die Ausführung von Tests wäre natürlich gut, aber dafür musst du die Tests natürlich erstmal hinzufügen. Die Tests werden sicher auch noch zum Verständnis der Anwendung beitragen.
Filter/DoctrineSQLFilter.php
Outdated
|
|
||
| public function addFilterConstraint(ClassMetadata $targetEntity, $targetTableAlias) | ||
| { | ||
| $this->assertSetUpCorrectly(); |
There was a problem hiding this comment.
Korrekt (glaube ich), aber lässt mich sprachlich stolpern.
|
@sebastiankugler @mpdude @FabianSchmick @MalteWunsch Was wäre eure Präferenz bezüglich Sprache? Die englischen Begriffe stammen vom Anfang, als ich noch den Größenwahn hatte, dass das ein riesiges Open-Source-Framework wird, das das Problem Sichtbarkeit von Entitäten in Symfony ein für alle Mal löst. Derzeit glaube ich, dass das vor allem wf-intern genutzt werden wird, es käm also wirklich auf die Präferenz des Teams an. |
|
Unsere Grundregel ist, Begriffe aus der Fachwelt des Kunden in deren Sprache zu verwenden, Framework-nahe Begriffe englisch (also z. B. isHauptrolle() statt istHauptrolle()). Das Konzept der Sichtbarkeit sehe ich an der Grenze zwischen Framework und Fachwelt. Die @visibility-Annotation gehört für mich zum Framework, sollte englisch sein. Eine konkrete Sichtbarkeitsregel könnte in der Fachwelt des Kunden leben, also SichtbarWennFreigeschaltet. Open Source-Repos wollen wir tendenziell komplett englisch halten. Alle Pakete, die wir nicht speziell für einen Kunden schreiben, wollen wir tendenziell open sourcen. Für mich ist die Unterscheidung nicht "Riesiges Open Source Framework" vs. "Kleine interne Library". Meine Hoffnung wäre, dass es eine kleine Open Source-Library werden könnte. Fazit: Ich würde hier im Repo alles englisch benennen, es sei denn, es handelt sich um eine Beispielimplementierung für eine kundenspezifische Sichtbarkeitsstrategie (und selbst dann könnte man noch für englisch argumentieren, weil es ja ein englisches Open Source-Repo würde). |
It is not the task of the "dependencies" action to validate well-formed PHP. It won't give you helpful information anyway - probably, it just treats non-PHP files as PHP files, no one knows, because it won't tell you in which file the parsing error is.
Fix coding standards in refs/pull/1/merge
…tered (Case 125045)
The testing pipeline depends on Symfony 4.4, and it's not worth porting it back to Symfony 3.4. See #6
…tDependencyInjector (Case 125045)
…ConsideringSQLFilter (Case 125045)
|
Halte den PR jetzt für mergebar. Ich bitte erneut um approval ;) @mpdude PS: Ich weiß, dass ich für VisibilityColumnConsideringSQLFilterTest besser die Doctrine Test Infrastructure genutzt hätte. Aber jetzt weiß ich wenigstens, warum ;) |
Made the assertions more confusing
Fix coding standards in refs/pull/1/merge
| use Webfactory\VisibilityFilterBundle\Filter\Strategy\FilterStrategy; | ||
| use Webfactory\VisibilityFilterBundle\Filter\VisibilityColumnRetriever; | ||
|
|
||
| class VisibilityColumnConsideringSQLFilterMock extends SQLFilter |
There was a problem hiding this comment.
Warum eine eigene Mock-Klasse?
Falls die Antwort: "weil VisibilityColumnConsideringSQLFilter final ist" lautet, würde ich in so einer Situation meistens überlegen, VisibilityColumnConsideringSQLFilter zu entfinalisieren oder (IMHO besser) ein Interface für VisibilityColumnConsideringSQLFilter einzuführen und das dann mit PHPUnit zu mocken.
There was a problem hiding this comment.
Ich bin generell der Meinung, dass die Klasse VisibilityColumnConsideringSQLFilter (wie die meisten Klassen) nicht Teil der Schnittstelle des Bundles sein sollte - nur haben wir im Moment keinen guten Weg, zu kommunizieren, welche Klassen Teil der Schnittstelle sind und welche nicht. Dann könnte man sich IMHO das final sparen. (Ich hätte übrigens nichts dagegen, wenn Klassen per default final wären - aber aktiv dafür sorgen zu müssen, lädt dazu ein, es vereinzelt zu vergessen, und wenn ich alle Klassen final mache außer eine, wirkt das IMHO wie eine Aufforderung zum Beerben - im Gegensatz dazu, wenn ich beerbare Klassen explizit kennzeichnen würde)
Hättest du auch Interesse daran, dass wir fürs Kommunizieren, ob Klassen Teil der Library-Schnittstelle sind, einen Weg finden, und soll ich das als Backender-Gesprächs-Thema auf eine Mittags-Meetings-Karte als Thema setzen?
There was a problem hiding this comment.
Um zum Thema zurückzukehren: Es gibt eine eigene Mock-Klasse, weil jeder Doctrine-Filter einen Klassennamen braucht. Ohne Klassennamen kann man Filter nicht registrieren, weil Doctrine sie intern erzeugt.
There was a problem hiding this comment.
Hättest du auch Interesse daran, dass wir fürs Kommunizieren, ob Klassen Teil der Library-Schnittstelle sind, einen Weg finden, und soll ich das als Backender-Gesprächs-Thema auf eine Mittags-Meetings-Karte als Thema setzen?
Die Frage wird selten aufgeworfen, ich weiß nicht, wie groß ich die Antwort machen wollen würde. Sie in offener Runde zu diskutieren, und eine neue Regel zu vereinbaren und lernen scheint mir übertrieben, ich würde mich aber auch nicht sperren. Ich persönlich hab mich bisher mit final/protected durchgewieselt, Symfony deklariert Klassen @internal und ich glaube auch @Api.
Fix coding standards in refs/pull/1/merge
There was a problem hiding this comment.
Sehr schön! 👏🏻 🙌🏻
Ich finde es toll, dass es Dir gelungen ist, das Problem jetzt doch auf ein ganz einfaches und damit gut verständliches Modell runterzubrechen. Die Probleme, die die Doctrine API Dir in den Weg gelegt hat, konntest Du ja letztlich doch ziemlich elegant umgehen.
Viele Kommentare sind nur Kleinigkeiten oder Anmerkungen, die m. E: nicht unbedingt zu Ändeurngen führen müssen. Bin auch so 👍.
Wichtige Punkte:
- Momentan ist die Filterregel, die angewendet wird, für alle Klassen immer die gleiche. Das sollte man aber m. E. problemlos erweitern können, wenn der Anwendungsfall kommt.
- Die Initialisierung über das
GetResponseEventwird nicht passieren, wenn man Console Kommandos ausführt.
|
|
||
| $filterCollection = $this->entityManager->getFilters(); | ||
|
|
||
| if (!$filterCollection->has(VisibilityColumnConsideringSQLFilter::NAME)) { |
There was a problem hiding this comment.
Wäre es auch möglich, das über die Klasse zu erkennen? Nur als Idee, ist auch so 👍.
| return $classMetadata->getColumnName($property->getName()); | ||
| } | ||
|
|
||
| private function getVisibilityProperty(ClassMetadata $classMetadata): ?ReflectionProperty |
There was a problem hiding this comment.
Klitzekleine Anmerkung: find... würde hier IMHO deutlicher machen, dass die in den $classMetadata gesucht wird, und dass es nicht etwa ein Feld (-> Getter) für diese Klasse selbst ist.
| */ | ||
| public function injects_dependencies_into_filter(): void | ||
| { | ||
| $this->entityManager->getConfiguration()->addFilter(VisibilityColumnConsideringSQLFilter::NAME, VisibilityColumnConsideringSQLFilterMock::class); |
There was a problem hiding this comment.
Ich verstehe, dass Du hier die Klasse VisibilityColumnConsideringSQLFilter brauchst, weil Du sie über den fest vorgesehenen Filter-Namen registrierst und damit später schauen kannst, ob der Filter richtig konfiguriert wurde. Ein "echtes" Mock-Objekt hat keinen vorhersehbaren Klassennamen (?) und kann wahrscheinlich auch nicht von Doctrine erzeugt werden.
Martin Fowlers Typologie (https://martinfowler.com/articles/mocksArentStubs.html) würde diese Klasse eher als "Spy" bezeichnen, nicht als "Mock".
Aber noch eine andere Überlegung:
Der Test hier ist sowieso schon ein KernelTestCase, d. h. ein Integrationstest, der ja schon darauf basiert, dass der OnRequestDependencyInjector richtig im DIC verdrahtet und konfiguriert ist.
Kannst du da nicht auch direkt (habe mir nicht überlegt, wie...) schauen, ob der VisibilityColumnConsideringSQLFilter korrekt eingerichtet wurde?
| use RuntimeException; | ||
| use Webfactory\VisibilityFilterBundle\Filter\Strategy\FilterStrategy; | ||
|
|
||
| final class VisibilityColumnConsideringSQLFilter extends SQLFilter |
| /** | ||
| * @test | ||
| */ | ||
| public function filter_gets_applied_on_findAll_method_of_repository(): void |
There was a problem hiding this comment.
Auf der einen Seite finde ich es schön, diese Tests hier einfach zu haben, weil sie dokumentieren, was passiert.
Auf der anderen Seite zeigen sie mit ..._on_findAll_..., on_custom_DQL, ... etc auch nur, was man doch eigentlich von Haus aus von einem Doctrine ORM Filter erwarten dürfte?
There was a problem hiding this comment.
Ich finde die Tests auch gut zur Dokumentation - bei Doctrine ist das alles nur sehr spärlich und abstrakt dokumentiert. Ich war teilweise selbst sehr gespannt, ob und wie die Tests funktionieren werden.
Ich würde mir sogar noch eher wünschen, dass es für den One-To-One-Fall noch einen Extra-Test mit Lazy-Loading gibt - denn wie sich der Filter bei One-To-One-Beziehungen mit und ohne Lazy-Loading verhält, ist AFAIK nirgendwo dokumentiert und kann nur geraten werden. Im Falle von Eager Loading ist die Beziehung nämlich null, mit Lazy Loading hat man erst mal ein Proxyobjekt und beim Versuch, es anzusprechen, eine NotFoundException.
Ich glaube auch nicht, dass Doctrine all diese Fälle durchtestet, entsprendend wären wir im Falle eines Falles ggf. die ersten, die es merken würden ;)
https://webfactory.fogbugz.com/f/cases/125045/jep-Ausbildungs-Abschlussprojekt-Stufenweise-Sichtbarkeit-ber-Doctrine-Filter
Bitte um ein Review :)