Return to Revisions

I think comments I made on your other open review around style, error handling, data validation, happy path, etc. are applicable here as well, so I won't re-hash them here.

$users = User::all();
$username = $request->input('usernameText');
foreach($users as $user) {
    if($user->Username == $username) {
        return redirect()->route('register');
    }
}

Are you really marshalling user objects for every user in your system to compare against provided username string one by one? This is really wasteful of resources and could actually bring your application server to it's knees with memory consumption as your user base grows.

At a minimum, you should query against the username field in your database (which should have a unique index on it) to specifically see if that user name exists (like you are doing in your authenticate() method). However, you should be thinking about going directly to insert (after validating input data of course) and simply checking to see whether the insert failed based on a unique index constraint on the username field. That is generally the most performant pattern for performing a unique index insert when you expect that there will be a low collision rate of input against existing records. This is because, with this approach, you always have exactly one query per execution as compared to a select-then-insert approach which would give you two individual queries against the database for most executions (with a single query in cases where there is a collision).

$keyspace =
'0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ!@#$%&';

This looks to me like a value that should be a class constant that is currently hidden away inside this method.

if(password_verify($request->input('passwordText'),$user->Password)) {

Invert this conditional so you can exit the method early on password mismatch.

Typically, one would need to regenerate session ID's at login/logout events. I admit I am not familiar with Laravel as as framework, so maybe that is happening here, but it does not seem to be.

I was unable to find documentation as to what \Session is referring to in Laravel, but it may be worthwhile exploring whether this class properly handles obsolescence of session data. Please see PHP session documentation below for really good information about how sessions should be handled securely at the lowest level in PHP.

http://php.net/manual/en/features.session.security.management.php

use Authenticatable, CanResetPassword;

Not sure if you have control over the names of these traits or not. If you do, you should ideally call them *Trait.

for($i = 0; $i < count($users); $i++) {
    if($users[$i]->Id == $user->Id) {
        unset($users[$i]);
        break;
    }
}

Again, rather than filtering out user in for loop, it seems like this should be filtered out in the query itself.

Even if you were to manually operate against this array of objects like you are doing, I don't understand why you would be unsetting the value at index, leaving a gap in your array's numerical index. Why not use array_filter() or array_splice() for this operation?

$str = '';
if(is_null($length) || !is_int($length) || (is_int($length) && $length < 0)) {
    $str .= 'random_str: Length is invalid. Length must be a positive integer. Value Provided: ' .
        var_export($length) . PHP_EOL;
}
if(strlen($keyspace) == 0) {
    $str .= 'random_str: Keyspace cannot be of length 0. Length must be a positive integer.' . PHP_EOL;
}
if(!empty($str)) {
    throw new InvalidArgumentException($str);
}

This seems an odd way to construct your exception messaging.

Why not simply:

if (empty($length) || !is_int($length) || $length < 0) {
    throw new InvalidArgumentException(...);
}
if (empty($keyspace) || !is_string($keyspace)) {
    throw new InvalidArgumentException(...);
}

Note that I have cleaned up some of the conditionals to be more appropriate as well. I don't think that you need to worry about messaging about exception for both parameters at once. This would be very unusual in practice, and not that meaningful anyway in debugging your application, as at the end of the day you still have an exception being thrown. If you solve the first error, and the second error is still not solved, it will immediately reveal itself.

It seems odd that in both of these pieces of code you have reviewed that this is the only place you are throwing an exception. I think if you commit to using exceptions (something I would strongly encourage), you need to be committed to using them throughout your codebase.

You are not consistent with multibyte string function usage in this method.