PHP OOP Login Script

Output of utility class

You are outputting error conditions and debugging statements to the output buffer on several occassions. This is an utility class, used by other code to log into a game. Error conditions should set a specific attribute that other code can use to detect an error and show an appropriate message such as "The server is currently under maintanance. Please try again later." when a database is not found, while you log useful information for developers only. Similarly, you can show a similar message when the player is, for example, already logged into the game.

Don't show sql errors to the user. They belong in a private error log somewhere. Give the user a user-friendly error message instead.

The ever-growing switch

In your constructor you have an ever-growing switch statement. These details do not belong in this class. You might choose to store them in their own table, or have a file with constants somewhere. You are setting yourself up for a world of hassle by duplicating login data, id's etc. over several files, then having to change them everywhere and potentially forgetting something.

Class naming

I don't think Login is a good name for this particular class. What you seem to try to emulate is a game session, so you probably should name it that instead.

Token generation

I am not sure what you are trying to accomplish with your token generation. uniqid() has two optional parameters, the first being a string containing a prefix, the second being a boolean to have more entropy. You seem desperate to get something unique, so you generate a random number between 0 and rand_max (which is then converted to a string for a prefix), and set "more entropy" to true, so you generate an even bigger token. Uniqueness is still not guaranteed though. Then you throw it through md5(), which is a hash function, which will thus increase the likelyhood that your token matches an other token.

You do not share why you are generating a token in the first place, or what problem you are trying to solve with it. If you just need a unique value to store game data with, why not use the player id? I assume a player has a saved game state per game, and can only play the same game one instance at a time. If you need to share a token with the client, why not use the session? The session token is already shared with the client, and you can just store your information in the session itself.

If you really need a token, try to use just uniqid("", TRUE); or uniqid("server-id", TRUE); if you are using some kind of load balancer set-up.

Orphaned sessions

Your login creates a game session, and stores it in the session variable. At no point you ever check if such a session already existed. You probably want to not automatically login in this class, but instead have both the login and logout procedures here. In the constructor retrieve any information you have on the current session, and let the login and logout procedure handle creation or destruction of the game session.

You can use this class in other operations too to check if the user is still logged in.

Use of comments

Don't use comments for single function calls. If your function names are to cryptic to understand what they are doing, you should rename the function. If a function has side-effects, you should rewrite it so those side-effects are eliminated. Otherwise, you are just stating the obvious. In other words: Don't do this.

//login to game
$this->login();

Whitespace

Your whitespace is mostly consistent. I would recommend adding a space after a comma, just like you would do in a normal sentence. The space makes argument lists a lot more readable.

date_default_timezone_set

I don't think this function call belongs in this class file. Instead set the default timezone in a config file. Consider using UTC instead of a random timezone somewhere in the world. You can convert this timestamp to a specific timezone if you need to display it to the user, hopefully based on some kind of preference.

This class is doing too much. Right now, this class:

• stores DB connection config
  • a concern that should really be moved out into application or, ideally, environmental configuration
• manages database connection instantiation
  • yet does nothing to promote connection reuse
• reads request headers
• manages player authentication
• outputs end user messaging to standard output

This is ripe for refactoring. You could ideally end up with 5-6 classes that individually handle their piece of the work, with concrete objects being appropriately passed as dependencies to the classes needing them.

For example, perhaps you need to pass a valid Game object to the constructor, with that object providing an already-instantiated mysqli object pointing towards the proper database host and schema. This would remove all responsibility from this class to worry about such things. It just gets a properly set up DB connection that it can work with. This allows you to pass around one game (with its config and DB connection) to many players.

Perhaps this class itself does not need to exist at all.

Based on what you have shown, your application cries out for a Player class. This player class might hold the concerns for authenticating a player (or perhaps there is some player factory or provider class that is used to instantiate Player objects).

With the existence of such a class, you might simply provide a means to attach different games (and thus different DB connections) to the concrete Player object in order to build the connections between players and games.

That might give you a usage pattern like this:

try {
    $player = new Player::getById($player_id);
    $player->login();
} catch (Exception $e) {
    // log error using whatever approach you app uses
    // for example...
    error_log($e->getMessage());
    // maybe rethrow or invoke end user messaging mechanism
}

// if successful...
try {
    $game = new Game($game_id);
    $player->addGame($game);
} catch (Exception $e) {
    ...
}

Some other code comments in addition to those noted in great answer by @Sumurai8

You should begin thinking about adopting use of Exceptions when writing your classes. They are one of the primary methods for notifying calling code of errors in trying to perform the caller's requests. They can be used to convey appropriate information to let the caller make the decision on how to handle the issue (i.e. pass it along for end user messaging, try to recover, etc.) as calling code is almost always in a better position to know how to handle the issue globally than is the code being called.

You are not validating any input to your public methods, thus leaving your class vulnerable to being put in a bad state (which for your class, as written, pretty much means the application will die, perhaps without useful trace of the problem being recorded in your logs). Before doing things like opening up database resources and the other things you are doing in your login() method, you should be first validating that passed parameters are as expected (a non-zero-length string and an integer. If this simple conditions are not met, you should ideally throw and exception or otherwise immediately stop executing the method.

Ideally if you take on the suggestion to refactor, you being to get in the habit of passing around object which are easier to validate as parameters via type hinting. This means that if the objects are set up properly before being injected as a dependency into the class, the class can fully rely on the functionality exposed by the dependency to work properly.

get_player() should perhaps be set_player(), as most typically associate get* terminology with having some value returned to caller.

        case 'game_001' :
            //set up variables
            $this->game_id = 001;

I am not understanding your game id naming approach here. Why not just use integer game id value? What value does having both game_*** and *** formats serve? Why would you be switching logic on "name" text string value versus a (typically) more authoritative id.

Use of 001 integer value here seems programmatically vague and potentially problematic. Why not just 1? What happens when there are 1000+ games?

    $client  = @$_SERVER['HTTP_CLIENT_IP'];
    $forward = @$_SERVER['HTTP_X_FORWARDED_FOR'];

Stay away from error suppression. empty() is your friend.