-
Notifications
You must be signed in to change notification settings - Fork 189
[Core] Fixing NDB_Client command line user instantiation #3212
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
[Core] Fixing NDB_Client command line user instantiation #3212
Conversation
| // Set user as unix username. | ||
| // Requires Loris to have user with this username | ||
| $user =& User::singleton(get_current_user()); | ||
| $user =& User::singleton(getenv('USER')); |
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.
I don't think $user is ever used. There is also an other affectation at line L#176. They should be removed.
If the only purpose of this is to crash when it fail, then a Exception/error message should be thrown.
Unless I am missing something...
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.
I updated the pull request description. Hopefully it makes sense. It's a somewhat simple, yet deep fix for a wrong exception that was thrown in the fix_timepoint_date_problems.php script. It's only supposed to crash when you try to run a script with a corresponding username that doesn't exist in LORIS(I'm sure there's a reason for this design...). Otherwise it shouldn't falsely crash when you're doing everything right.
You're right about $user not being used. #3216
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.
getenv("USER") is better than get_current_user() for the intended purpose. Still, we should remove those lines and use lazy instanciation instead of this "effet de bors"
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.
@xlecours pretty sure thats to allow the DB to trackchanges when you run backend scripts in the history tables. it uses the unix user as lorisuser.
its annoying because it requires you to have a loris user with the same userid as ur unix user. and when it comes to
get_current_user it was actually checking the owner of the PHP file rather than the actual logged in unix user..
I'm not 100% on it but @driusan should be able to tell if its true or not.
| // Set user as unix username. | ||
| // Requires Loris to have user with this username | ||
| $user =& User::singleton(get_current_user()); | ||
| $user =& User::singleton(getenv('USER')); |
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.
getenv("USER") is better than get_current_user() for the intended purpose. Still, we should remove those lines and use lazy instanciation instead of this "effet de bors"
Correctly calling User singleton with the accurate username
This came up because when fix_timepoint_date_problems.php calls User::singleton, it passes the current unix user as an argument but it's pretty useless since the static instance is already being initialized at the beginning. Plus the code is incorrectly using the "owner of the file" as the argument. This also fixes the inaccurate error message produced by fix_timepoint_date_problems.php.
We can also merge #3216 once this is merged.