prevent errors by making the signature of Deputy::isDeputy() more fault-tolerant, fixes #1718

assigned to @tleilax

@elmar @anoack was sagt ihr?

Wieso ist das denn nun - zwei Jahre nach der ursprünglichen Änderung - ein besonderes Problem?

Praktisch alle Fälle dieses Fehlers, die ich in der Zeit gesehen habe, rufen get_studip_perm() mit NULL als ID einer Veranstaltung (bzw. Einrichtung) auf und laufen dann indirekt in diesen Fehler, sofern die Vertretungsfunktion aktiviert ist. Ich kann mich nur an einen Fall erinnern, wo isDeputy() direkt falsch verwendet wurde - da waren allerdings die beiden Parameter im Aufruf vertauscht und es gab keine Fehlermeldung (es funktionierte einfach nur nicht).

Meine Meinung dazu ist:

• Wer isDeputy() falsch aufruft, darf ruhig einen deutlichen Fehler bekommen.
• Wenn wir möchten, daß get_studip_perm() ohne Veranstaltung aufgerufen werden darf und dann einfach immer false liefert, sollten wir das direkt in dieser Funktion tun. Und zwar unabhängig davon, ob die Vertretungsfunktion aktiviert ist.

Ob letzteres sinnvoll ist, hängt davon ab, wie nachlässig der Rest des Systems programmiert ist, und da hatte ich den letzten Monaten praktisch keine Probleme mehr gesehen. Der Fall in #1576 (closed) wäre für mich jetzt kein Grund: Da wurde wohl die Verwaltungsseite einer gelöschten Studiengruppe aufgerufen, und ob das Verhalten aus Nutzersicht "besser" wird, wenn dieser Fehler ignoriert wird, ist überhaupt nicht klar - ggf. läuft man danach in einen anderen Fehler.

Mich stört hier aber generell, daß diese Art von Fehler nur auffällt, wenn die Vertretungsfunktion aktiviert ist. Viele Entwickler haben die nicht angeschaltet und merken nicht, wenn sie so einen Fehler eingebaut haben - es ist ja nicht offensichtlich kaputt. Insofern wäre ich dafür - falls wir diesen Fehler weiterhin berichten wollen - das auch schon direkt in get_studip_perm() abzufangen und die Übergabe von NULL direkt dort als Fehler zu melden.

Noch eine Ergänzung dazu: Mit der Änderung hier würden Fehler wie #1044 (closed), #937 (closed) oder #1047 (closed) ggf. erst mal gar nicht auffallen, bis jemand bemerkt, daß die eigentlich eingebaute Rechteabfrage manchmal nicht funktioniert - gerade da ist ein deutlicher Fehler, der auch über die JSON-API sichbar zurückgemeldet wird, aus meiner Sicht ein Gewinn.

Wie gehen wir hier weiter vor?

@elmar @tleilax

Von mir aus können wir das einfach verwerfen und wir fixen dann halt jede Stelle, an der der Fehler auftritt. Meine Intention war bloß, dass wir das Verhalten von vorher wieder herstellen, weil's unkritisch ist und wir dann nicht in Plugins wieder drüber stolpern.

Alles klar

closed