Conversation
j3nsch
left a comment
j3nsch
left a comment
There was a problem hiding this comment.
Vielen Dank! Ich denke wir müssen über ein paar Punkt noch einmal nachdenken, damit sich der DocumentFinder wie erwartet verhält. Evtl. müssen wir auch das Interface bzw. seine Dokumentation noch erweitern.
| public function getIds() | ||
| { | ||
| return $this->finder->ids(); | ||
| $this->select->select("d.id"); |
There was a problem hiding this comment.
Warum Double-Quotes? Bitte nur bei Bedarf.
| public function setDocumentIds($documentIds) | ||
| { | ||
| $this->finder->setIdSubset($documentIds); | ||
| // Hotfix: If $subset is empty, return empty set. |
There was a problem hiding this comment.
Nein, ich denke nicht. Warum diesen "Hotfix"? Ein leeres Array ist offensichtlich ein Fehler, ein falscher Parameter und sollte entsprechend behandelt werden, ansonsten verstecken wir hier Probleme.
There was a problem hiding this comment.
Gibt es einen Test der dieses Verhalten erfordert? Wenn es irgendwo Code gibt der versucht Dokumente ein einer leeren Menge von möglichen Dokumenten zu finden, dann stimmt was nicht mit diesem Code.
There was a problem hiding this comment.
Den Hotfix habe ich entfernt. Einen Test der diesen erfordert konnte ich nicht entdecken.
| public function setBelongsToBibliography($partOfBibliography = true) | ||
| { | ||
| $this->finder->setBelongsToBibliography($partOfBibliography); | ||
| $this->select->andWhere('d.belongs_to_bibliography = :setBelongsToBibliography_partOfBibliography') |
There was a problem hiding this comment.
Das sind sehr lange Parameternamen. Dadurch, dass der Funktionsname mit drin steckt, kann man die Parameter beim Debugging vielleicht leichter zuordnen. Das finde ich gut. Der Rest ist, zumindest manchmal, ein wenig redundant. Hier würde ich den Namen zu setBelongsToBibliographyParam ändern. Meinetwegen auch mit Unterstrich. Der kürzere Name liest sich einfacher.
Klar wenn in einer Funktion mehr als ein Parameter verwendet wird, sollte der Name noch weiter differenziert werden.
There was a problem hiding this comment.
Also setDocumentIdRange_start und _end machen z.B. Sinn und können so bleiben oder z.B. in setDocumentIdRangeStart oder setDocumentIdRangeParamStart umgewandelt werden. Einheitlichkeit ist schön und wichtig, aber weit davon entfernt unser wichtigstes Problem hier zu sein. So etwas wie setCollectionId_collectionId ist einfach unnötig und sollte in setCollectionIdParam umgewandelt werden. Danke!
| $subSelect = $queryBuilder->select('i.id') | ||
| ->from('document_identifiers', 'i') | ||
| ->where('i.document_id = d.id') | ||
| ->andWhere('type = :setIdentifierExists_name'); |
There was a problem hiding this comment.
Das Problem, dass ich hier sehe ist, dass es sinnvoll sein kann die Funktion zweimal aufzurufen. Das hat in der alten Implementation funktioniert. Ein Dokument müsste dann Identifier für beide Namen haben. In der neuen Implementation wird in beiden Fällen der gleiche Parameter-Name verwendet, so dass das vermutlich hier nicht mehr funktionieren würde.
Ich nehme an für diese Nutzung gibt es keine Tests. Diese sollten ergänzt werden. Da es in der Vergangenheit einfach "automatisch" funktioniert hat, hat sich vermutlich niemand weiter Gedanken über Tests gemacht.
There was a problem hiding this comment.
Wie könnte man das machen? Vielleicht mit einem Counter, so dass an den Parameternamen noch eine Nummer dran gehängt wird? Oder hat eine Lösung, bei der mit jedem Aufruf die zusätzlichen Bedingungen in einem Array erfasst werden und das Query-Objekt erst am Ende beim Aufruf von getYYYY gebaut wird, mehr Vorteile?
There was a problem hiding this comment.
Es gibt jetzt eine Funktion die mittels Counter eindeutige Parameter-Namen erzeugt. Einen exemplarischen Test für die Funkrion setIdentifierValue habe ich hinzugefügt.
| */ | ||
| public function testCountOnEmptyDb() | ||
| { | ||
| $finder = new DefaultDocumentFinder(); |
There was a problem hiding this comment.
Verschiebe das mal bitte ins Setup, damit wir die Instanzierung nur an einer Stelle haben. Spricht etwas dagegen?
There was a problem hiding this comment.
Vielleicht macht auch eine createDocumentFinder Funktion Sinn, damit man innerhalb eines Tests mehrere Finder verwenden kann.
There was a problem hiding this comment.
Da einige Tests auch mehrere Finder verwenden habe ich eine Create-Funktion angelegt.
| */ | ||
| public function testIdsByState() | ||
| { | ||
| // $this->markTestSkipped('TODO DOCTRINE DBAL Issue #129'); |
There was a problem hiding this comment.
Kein skip, habe ich entfernt.
| /** | ||
| * Extended functionality: Grouping | ||
| */ | ||
| public function testGroupedDocumentTypes() |
There was a problem hiding this comment.
Funktion ist heißt jetzt getDocumentTypes.
| $this->assertTrue(in_array($doc3->getId(), $resultDocIds), 'Expected Document-ID in result set'); | ||
| } | ||
|
|
||
| public function testSetFilesVisibleInOai() |
There was a problem hiding this comment.
Funktion heißt jetzt setHasFilesVisibleInOai.
|
|
||
| public function testFindDocumentsForXMetaDissPlus() | ||
| { | ||
| $this->markTestSkipped('TODO DOCTRINE DBAL Issue #129'); |
There was a problem hiding this comment.
Hier müssen ebenfalls neue Funktionen verwendet werden: setServerState und setDocumentType anstelle von
setServerStateInList und setTypeInList.
| */ | ||
| public function testFindDocumentsWithExpiredEmbargoDateForUpdatingServerDateModified() | ||
| { | ||
| $this->markTestSkipped('TODO DOCTRINE DBAL Issue #129: Function setEmbargoDateBeforeNotModifiedAfter() is no part of the DocumentFinderInterface'); |
There was a problem hiding this comment.
Die Funktion wurde aufgeteilt bzw. jetzt müssen setEmbargoDateBefore und setNotModifiedAfterEmbargoDate verwendet werden. Der Test sollte dahingehend umgebaut werden, so dass die Idee, die Funktionalität weiterhin getestet wird.
There was a problem hiding this comment.
In den betroffen Tests habe ich jetzt die neuen Funktionen verwendet.
|
|
||
| $today = date('Y-m-d', time()); | ||
|
|
||
| $doc = new Document(); |
There was a problem hiding this comment.
Bitte auf Document::new() umstellen.
There was a problem hiding this comment.
Habe ich umgestellt.
…ForXMetaDissPlus.
… using new directly.
…setNotInXmlCache.
2 participants
PR for #129