Superglobals In Classes: Revisited

Last December, I wrote about the use of PHP superglobals inside of classes (link here). I asserted at the time that superglobals inside of a class violated some basic rules on what a class was supposed to do. Today, I am revisiting that discussion.

The placement of superglobals inside a class creates an impossible situation for code reuse. Take for example my original sample:

<?php

class VerifyLogin extends UserObject
{
	function verifyCredentials()
	{
		$username = mysql_real_escape_string($_POST&#91;'username'&#93;);
		$password  = mysql_real_escape_string($_POST&#91;'password'&#93;);
		$passwordhash = MD5($username . $password); // Salt our PW Hash
		$sql = 'SELECT id FROM userTable WHERE username = ' . $username . ' AND password = ' . $passwordhash . ' LIMIT 1';

		$resource = mysql_query($sql);

		if(mysql_numrows($resource) > 0)
		{
			$array = mysql_fetch_row($resource);
			$this->userID = $array[0];
			$this->loggedIn = true;
			return true;
		}
		else
			return false;
	}
}

?>

This code will work just fine for our purposes – on this site. But what happens when we want to move this to another site? Unless we leave our form fields named “username” and “password” we’ll have to modify the original code. This makes reusing the code more difficult. Since user validation is particularly frequent, there’s no reason why we should be rewriting this code each and every time we create a new application.

There is a way to refactor this code to make it easier to reuse later on (note that my re-factoring simply is addressing superglobals, and no other facet of the code):

<?php

class VerifyLogin extends UserObject
{
	function verifyCredentials($username, $password)
	{
		$passwordhash = MD5($username . $password); // Salt our PW Hash
		$sql = 'SELECT id FROM userTable WHERE username = ' . $username . ' AND password = ' . $passwordhash . ' LIMIT 1';

		$resource = mysql_query($sql);

		if(mysql_numrows($resource) > 0)
		{
			$array = mysql_fetch_row($resource);
			$this->userID = $array[0];
			$this->loggedIn = true;
			return true;
		}
		else
			return false;
	}
}

$ValidUser = new VerifyLogin($_POST['username'], $_POST['password']);

if($validUser)
{
	// Do something here
}
else
{
	// Do something else
}

?>

This refactored code means that we can use our class anywhere we like, for anything we like, without having to really care what our $_POST array looks like. We simply have to supply the data to the method, and it will do the work for us.

Object-oriented programming in PHP allows us to do amazing things like build frameworks and libraries that can be used over and over again. We shouldn’t muck that up by including short-term or application-specific functions and variables when they’re unnecessary.

Brandon Savage is the author of Mastering Object Oriented PHP and Practical Design Patterns in PHP

fqqdk (@fqqdk) wrote at 7/13/2009 7:26 pm:

Why did the verifyCredentials() method turn into a constructor in the second snippet? It servers no purpose for the topic (superglobals in methods). It’s confusing. Constructors should not do work. What does returning boolean values from a constructor do?

Aside of these things what you say is good and correct. Global state is evil. :)

Brandon Savage (@brandonsavage) wrote at 7/13/2009 7:31 pm:

You’re right; I’ve fixed that error. Constructors cannot return things.

Jason Stubbs wrote at 7/13/2009 7:39 pm:

The biggest “security hole” is that $validUser will always evaluate to true as it becomes an instance of VerifyLogin regardless of the constructor’s return value… To make your point explicit, you should probably have the original verifyCredentials() function to take $username and $password as parameters.

Your point itself though is completely valid and I’d even extend it to say that classes should not use _any_ global variable at all. Everything should be either passed in via a constructor/function or, in the case of cross-object internal data, stored in a static class variable.

Brandon Savage (@brandonsavage) wrote at 7/13/2009 8:06 pm:

Your comment was just a bit behind my correction; you’re 100% right that a constructor returns an object regardless of a return statement.

Daniel O'Connor (@clockwerx) wrote at 7/13/2009 9:09 pm:

I’d still do this differently, because you’ve still got global state.

class DBAuthenticator {
/** @param resource $db The database connection to use, which is a dependency */
public function __construct($db) {}

/**
* @return int User ID
* @throws Exception No such user
* @throws Exception Invalid login
*/
public function authenticate($user, $pass) {}
}

class User {
public $id;
}

// Setup
$user = new User(); // Your model/data structure
$storage = new DBStorage(); // A layer to load and populate your models
$security = new DBAuthenticator($db_connection);

// Wiring
$user->id = $security->authenticate($_POST[‘user’], $_POST[‘pass’]);
$user = $storage->load($user);

That neatly splits:
* The authentication method (“Are these credentials allowed to access my resources”) from the user profile
* The authentication backend (database) from the user profile model

Changing the security mechanism from DBAuthenticator to MockAuthenticator or LDAPAuthenticator is then trivial, too.

Christer (@cogocogo) wrote at 7/14/2009 1:40 am:

You should probably fix the security holes as well since many beginners might use the code anyway, regardless of the warning in the bottom of the post, and they wouldn’t have any knowledge on how to fix the security flaws…

Artur Ejsmont wrote at 7/14/2009 4:18 am:

Although refactoring makes sense and code seems better now, you are not escaping user name any more …. its like asking for injections.

Art

Dave (@prionson) wrote at 7/14/2009 6:21 am:

You’ve just switched your dependancy from the global variable to an SQL table leaving the fundamental problem unaddressed.

The real problem is that the OO model you’ve chosen isn’t very sensible. Why should verifyLogin be an object? Personally I would have an AuthenticationHandler which is responsible for maintaining the global state. This could be an interface with site specific implementations but it maps much better to the necesary global state because you can at least make it a singleton.

Gyorgy (@feketegy) wrote at 7/14/2009 10:30 am:

Good point, I’d like that every developer would use OOP powers to the max, because
superglobals in classes renders useless one of the basic principles of OOP (code isolation)

fqqdk (@fqqdk) wrote at 7/17/2009 9:41 am:

This snippet still has some problems…

$ValidUser = new VerifyLogin($_POST[‘username’], $_POST[‘password’]);
if($validUser) //…

First: now that you modified the method to not be a constructor, the construction could should be updated too:
$ValidUser = new VerifyLogin();
//…
Second: you have the variable name starting with different cases: you have $ValidUser assigned to your VerifyLogin instance and you check for $validUser in the conditional.

Third: after you fix the variable name, the condition will always evaluate to true because $ValidUser is an object, and that would impose some very serious security problems :D The condition should be spelled
if($ValidUser->verifyCredentials($_POST[‘username’], $_POST[‘password’]) //…

Fourth: even the variable name is misleading. After moving the authentication logic from the constructor to the verifyCredentials method $ValidUser does not at all denote a valid user! That name should be changed.

And the last problem is what Dave already mentioned, I will not repeat him. This problem is related to what you write in your recent article (the one about multiple levels of inheritance). The authentication process should be separated from the actual representation of users. The statement VerifyLogin IS A User does not make sense at all!

I would advise that you review your articles more thoroughly. Clueless noobs may find them, and without the same background as seasoned veterans they might take your code or worse your flawed concepts for granted, and replicate it in their designs! I actually found this post through the feed of Planet PHP, and given that popularity is power, the Spiderman principle should apply :)

Copyright © 2023 by Brandon Savage. All rights reserved.