heimrichhannot / contao-utils-bundle

This bundle offers various utility functionality for the Contao CMS.
GNU Lesser General Public License v3.0
8 stars 4 forks source link

Added methods: getActiveGroups and hasActiveGroups to memberUtil und userUtil #26

Closed AlexejKossmann closed 3 years ago

koertho commented 3 years ago

Grundsätzlich Frage: Sollten diese Methoden nicht lieber ins member-bundle und das Member-Model bzwl User-Model erweitern?

Defcon0 commented 3 years ago

Das MemberModel/UserModel erweitern ist gar keine gute Idee wegen DC_Multilingual. Die Models sind laut Leo Feyer nicht anpassbar. Ich würde es im utils-bundle sehen, da das member bundle wieder sehr viel ballast mitbringt, den man nicht braucht.

Defcon0 commented 3 years ago

Die Funktionen sind allgemein genug da tl_member und user Core sind.

koertho commented 3 years ago

Was DC_Multilingual anbelangt, wäre hier egal, da es keine statischen Methoden wäre, sondern konkrete Instanz-Methoden. Was die Erweiterbarkeit der Models anbelangt, ok, gehe ich mit. Ballast, naja, anderes Problem :D

@AlexejKossmann Hast du ein wenig recherchiert, ob es sowas eventuell schon vom Core gibt?

AlexejKossmann commented 3 years ago

@koertho ja, habe aber nichts gefunden was ich nutzen kann

koertho commented 3 years ago

Ich fände es gut, wenn du noch Tests für die neuen Methoden anlegst. Wenn du die Test-Klassen anlegst, kannst du dich an den neuen Testsklassen im Utils-Namespace meines offenen PRs orientieren, vor allem die getTestInstance()-Methode ist hier zu beachten. Hier könnte ich mal noch eine Abstrakte Klasse bauen. Mhm :thinking:

koertho commented 3 years ago

Eine weitere Anmerkung: Die Contao-User-Klasse hat eine isMemberOf-Methode. Bitte hier mal gegenprüfen, außerdem löst diese die Gruppen bereits als Array auf. Eventuell sparen wir uns da doppelten Code ;)

Defcon0 commented 3 years ago

Witzigerweise wird die Methode im gesamten Contao nicht verwendet und prüft nicht auf disable 👍 Aber du könntest die Methode als Grundlage nehmen.

Defcon0 commented 3 years ago

Ah, ich hab nix gesagt. arrGroups enthält nur die aktiven :)

koertho commented 3 years ago

Nachteil an der User-Klasse: geht nur für den aktuell eingeloggten Nutzer, ist eine Singlton :roll_eyes:

Defcon0 commented 3 years ago

Dann bitte doch die eigene Funktion Alexej...

koertho commented 3 years ago

Habe jetzt noch eine Lösung gefunden, da ich gerade an einer ähnlichen Thematik sitze. Es gibt den contao.security.frontend_user_provider service (und entsprechend auch für backend user), damit bekommt man eine User-Klasse auch für andere Nutzer. Natürlich ist das nirgendwo dokumentiert und ich weiß nicht, ob es das schon in 4.4 gibt.

AlexejKossmann commented 3 years ago

@koertho ich kann hier den ContaoUserProvider nicht benutzten da, in contao 4.4 nur der aktuelle Nutzer genommen wird, mit diesen Methoden soll aber generell jeder Nutzer gecheckt werden. in 4.9 muss außerdem unterschieden werden ob der FrontendUserProvider oder der BackendUserProvider benutzt werden muss, das führt zu vielen service definitionen die ich eigentlich nicht brauche, da ich trotzdem den modelUtil nutzen muss um zu prüfen ob die nutzergruppe activ ist, denn es kommen im Nutzer einfach alle gruppen zurück ohne Unterscheidung

koertho commented 3 years ago

@AlexejKossmann Hä? Es gibt zwei Services, einen für BackendUser und einen für FrontendUser (genauso wie es hier im PR ist). Die kommen von Contao, da musst du gar nichts selber definieren. In Contao 4.4 gibt es die auch schon, habe sie da allerdings nicht getestet. @Defcon0 hat doch schon geschrieben, dass nur aktive Gruppen drin sind.

AlexejKossmann commented 3 years ago

@koertho wir können leider den ContaoUserProvider nicht benutzen um an die aktiven Gruppen zu kommen.

Folgende Probleme gibt es:

https://github.com/contao/contao/blob/5fd76d55ab9266027585eea6d7ace08730af49a7/core-bundle/src/Resources/contao/library/Contao/User.php#L459-L467 Es muss dabei einen Request geben, der im Browser ausgeführt wird. Wenn es z.B. ein Command ist wird das alles nicht functionieren.

https://github.com/contao/contao/blob/4.9/core-bundle/src/Resources/contao/classes/BackendUser.php#L405-L407 hier wird der Globale Benutzer überschrieben. Also falls ich damit einen anderen Nutzer als den aktuell eingeloggten suche wird wird dieser umgeschrieben.

https://github.com/contao/contao/blob/5fd76d55ab9266027585eea6d7ace08730af49a7/core-bundle/src/Resources/contao/classes/BackendUser.php#L438-L458 hier werden nur die permissions des Benutzers zusammengetragen, nicht die activen Gruppen, die gruppen bleiben alle trotzdem da, ob active oder disabled

ps: das könnte auch auf deine Implemention einen Einfluss haben

Defcon0 commented 3 years ago

@AlexejKossmann Die Links führen irgendwie zu 404.

AlexejKossmann commented 3 years ago

@Defcon0 Danke, habs aktualisert

koertho commented 3 years ago

@AlexejKossmann Da stimmte ich dir zu, das sieht nicht gut aus. Sehr schade, dann benötigt es diesen PR wohl doch :)

AlexejKossmann commented 3 years ago

@Defcon0 Habe die methoden etwas umgeschrieben. Die $table aus den properties ist weg, habe es durch constanten ersetzt