Task #38997

Task #38984: Code Review

Fix of extension code review

Added by Tizian Schmidlin over 9 years ago. Updated over 9 years ago.

Status:
Closed
Priority:
Must have
Category:
TYPO3 4.x
Start date:
2012-07-16
Due date:
% Done:

100%

Estimated time:
5.00 h
Spent time:
#1

Updated by Tizian Schmidlin over 9 years ago

class.tx_phoenixlogin_logincall.php:
l. 62:
//TODO: It would be more readable if you would call userExists above the if statement, and check with the === operator if the method has returned false. Or use brackets like this > !($this>user = $this->userExists())
// DONE, thank you for pointing this out.

l. 75:
//TODO: Why are the methods userExists() and generateRandomPassword() public? If they're not needed outside this class, make them private
/ As they might be used in further purpose from outside and from other methods and since they do not return critical data, they might as well be
// public I think. Maybe the generateRandomPassword() should be declared as static, this would make more sense. Likely, I'd declaire these functions
// as protected and not private, since they could not be extended for further user or adaptation if declared as private.

l.167:
// TODO: I'm not an expert on cryptography, but since you're using a deterministic pseudorandom number generator the password may be predictable. But for our purposes this should not be a problem.
// Enhancement: i now randomly add lowercase characters and use mt_rand so that the randomnessy is improved.
// Still, with bruteforcing you might get the password but it might take you a million years (the password is also hashed when saved to the database, so the risk is even lowered).
// Also improved performance by using for() instead of each, since we have a defined number of iterations

l.189:
// TODO: Remove all comments that are not needed like the one below
// DONE

#2

Updated by Tizian Schmidlin over 9 years ago

  • % Done changed from 0 to 100
#3

Updated by Tizian Schmidlin over 9 years ago

  • Status changed from New to Closed
#4

Updated by Tizian Schmidlin over 9 years ago

Fixed this too:
//TODO: You don't need the authenticationManager anywhere in the code. So you can remove it safely.

And addded the spent time...

Also available in: Atom PDF