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

be_relation sortierbar machen. #94

Closed
wants to merge 2 commits into from
Closed

be_relation sortierbar machen. #94

wants to merge 2 commits into from

Conversation

alxndr-w
Copy link
Contributor

#89

Änderungen waren in R4 xform schon von mir getestet, aber bitte gerne nochmal drüber schauen. @gharlan

GitHub für Windows hat die Änderung nicht vor dem PR übernommen.
@alxndr-w
Copy link
Contributor Author

@dergel soll ich das noch mal testen?

Fände es gut, wenn's im nächsten Release ist, weil ich das selbst für ein Projekt brauche und gerne fest verbaut sehen würde und das nicht durch Updates verloren gehen soll.

@gharlan
Copy link
Member

gharlan commented Jul 13, 2016

Ich sehe hier zwei Probleme:

  • Der PR geht davon aus, dass Mysql die Datensätze in der Reihenfolge herausgibt, wie sie eingefügt wurden. Das ist zwar in der Regel so, muss aber nicht immer so sein. Die Default-Sortierung bei SQL ist explizit unbestimmt. Habe es meine ich auch schon anders erlebt.
  • Ich kann mir auch Szenarios vorstellen, wo es in der Verknüpfungstabelle noch weitere Felder gibt, die optional zusätzlich gesetzt werden können (Dazu muss man dann allerdings explizit in die Verknüpfungstabelle gehen). In meinen Nicht-Redaxo-Projekten hab ich regelmäßig weitere Felder in den n:m-Verknüpfungstabellen. Diese Felder gehen mit diesem PR verloren, da immer neu eingefügt wird.

In der Regel wird es aber wohl funktionieren, und ob der zweite Fall wirklich mal so vorkommt, ist auch fraglich. Löst man in yform vermutlich eher irgendwie anders.
Daher bin ich auch nicht böse, falls Jan sich trotzdem für's Mergen entscheidet. :D

@alxndr-w
Copy link
Contributor Author

Danke Gregor für die Erläuterung 👍

Zu deinen Punkten:

Default-Sortierung: Dann müsste man auch davon ausgehen, dass bestehende Einträge komplett durcheinander gewürfelt werden, wenn man sie nochmals bearbeitet.

Datensätze erhalten: In Abwägung hätte ich gehofft, dass "Reihenfolge der verknüpften Datensätze ändern" allgemein wichtiger ist und eher dem Standardverhalten des Feldes in 99% der Fälle entspricht. Wer es anders benötigt, kann ja wiederum einen eigenen Feldtyp schreiben.

Wenn man einen Datensatz anlegt und andere Datensätze in be_relation verknüpft, kann man ja aktuell vor dem Speichern des Datensatzes sogar noch die Reihenfolge festlegen. Nach dem Speichern nicht mehr. Das ergibt einfach keinen Sinn.

Ich drück mir selbst jetzt fest die Daumen ;)

@gharlan
Copy link
Member

gharlan commented Jul 13, 2016

Default-Sortierung: Dann müsste man auch davon ausgehen, dass bestehende Einträge komplett durcheinander gewürfelt werden, wenn man sie nochmals bearbeitet.

Ja. In der aktuellen Variante sind die Sortierpfeile falsch, da es eigentlich gar keine Sortierung gibt, nur eine Zuordnung. Sind die Datensätze verknüpft, oder nicht.

Um es ganz sauber, mit Sortierung zu lösen, bräuchte man mMn. eine Prio-Spalte in der Verknüpfungstabelle. Bzw. eigentlich sogar zwei, jeweils eine für die beiden verknüpften Tabellen.

Denn mir fällt noch ein Problem bei deiner Variante auf:
Angenommen wir haben Tabelle A und B, und Relationstabelle A_B. Wenn die Zuordnungen sowohl in A, als auch in B gesetzt werden können, geht jeweils die Sortierung im anderen Datensatz verloren.
Also ich gehe in einen A-Datensatz, wähle dort die B-Datensätze aus und sortiere sie, und speichere.
Nun gehe ich in einen der B-Datensätze und sortiere dort die A-Datensätze um und speicheren.
Dann ist die Sortierung in dem A-Datensatz verkehrt, da der Verknüpfungsdatensatz neu angelegt wurde.

@alxndr-w
Copy link
Contributor Author

alxndr-w commented Jul 13, 2016

Stimmt. Mist. (Wobei das Problem nicht durch den PR entsteht, sondern auch jetzt schon da ist.)

Sollen wir die Grundsatzdiskussion in das entsprechende Issue packen?
#89

@dergel
Copy link
Member

dergel commented Sep 28, 2016

ich schliesse den PR und im issue gehts dann weiter

@dergel dergel closed this Sep 28, 2016
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.

None yet

3 participants