-
-
Notifications
You must be signed in to change notification settings - Fork 554
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
Code review #4469
Code review #4469
Conversation
nilsteampassnet
commented
Nov 16, 2024
- background_tasks___userKeysCreation.php: comments translations ; Ignored an error in Scrutinizer
- task_maintenance_purge_old_files.php: Fixed an issue regarding folder ressource
- identify.php: Fixed Scrutinizer warning, missing variables initialization
- logs.datatables.php: fixed possible type concatenation
- tree.php: improved recursive tree function
- Removed some unused variables
- Added Scrutinizer ignore
- Reworked function storeUsersShareKey() based upon Scrutinizer findings
* Added Scrutinizer ignore * Reworked function storeUsersShareKey() based upon Scrutinizer findings
…nored an error in Scrutinizer * task_maintenance_purge_old_files.php: Fixed an issue regarding folder ressource * identify.php: Fixed Scrutinizer warning, missing variables initialization * logs.datatables.php: fixed possible type concatenation * tree.php: improved recursive tree function
'error' => true, | ||
'message' => $lang->get('error_bad_credentials'), | ||
'error' => false, | ||
'user_info' => $userADInfos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessarily a problem but I wonder what exactly $userADInfos
contains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember well, $userADInfos contains a set of LDAP user attributes such as DN, mail, givenname, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor changes and it should be okay, nice job!
* Fixed ordering not working
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!