Skip to content

Conversation

@Jkat
Copy link
Contributor

@Jkat Jkat commented Oct 25, 2017

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.

@Jkat Jkat added Category: Bug PR or issue that aims to report or fix a bug [branch] bugfix labels Oct 25, 2017
@Jkat Jkat added this to the 18.0.3 milestone Oct 25, 2017
@Jkat Jkat changed the title [Core] Fixing NDB_client command line user instantiation [Core] Fixing NDB_Client command line user instantiation Oct 25, 2017
// Set user as unix username.
// Requires Loris to have user with this username
$user =& User::singleton(get_current_user());
$user =& User::singleton(getenv('USER'));
Copy link
Contributor

@xlecours xlecours Oct 25, 2017

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...

Copy link
Contributor Author

@Jkat Jkat Oct 26, 2017

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

Copy link
Contributor

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"

Copy link
Collaborator

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'));
Copy link
Contributor

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"

@driusan driusan merged commit 6bde669 into aces:bugfix Oct 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Category: Bug PR or issue that aims to report or fix a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants