Changing class to allow for variables in constructor

I have the following class:

class User {

   private static $instance;
   private $error;
   // private $validuser = 0;
   private $username;
   private $password;
   public $loggedin = false;
   public $userlanguage = 163;   

   private function __construct($group = null) {
     $core = Core::getInstance();
     $this->userid = (isset($_SESSION['userid']) ? $_SESSION['userid'] : '');

     foreach (Config::readAll() as $key => $value) {
       $$key = $value;
     }

     $get_user = $core->dbh->prepare("SELECT t1.*,t1.user_id AS t1userid,t2.*,t3.*,t3.type_id AS t3typeid,t3.group_id AS t3groupid,t4.*,t4.name AS t4name,t5.*,t5.name AS t5name,t6.*,t6.group_id AS t6groupid,t6.active AS groupActive,t7.*,t7.name AS t7name,t9.* FROM persdb_users t1 LEFT JOIN persdb_user_settings t2 ON t1.user_id = t2.user_id LEFT JOIN persdb_user_types t3 ON t1.user_id = t3.user_id LEFT JOIN persdb_usertypes t4 ON t3.type_id = t4.id LEFT JOIN persdb_roles t5 ON t2.role_id = t5.role_id LEFT JOIN (SELECT * FROM persdb_user_groups WHERE active = 1) AS t6 ON t1.user_id = t6.user_id LEFT JOIN persdb_groups t7 ON t6.group_id = t7.group_id LEFT JOIN persdb_countries t9 ON t1.nationality = t9.id WHERE t1.user_id = :userid GROUP BY t7name ORDER BY t6groupid");

     if ($get_user->execute(array(':userid'=>$this->userid))) {
       $g_user = $get_user->fetch(PDO::FETCH_ASSOC);
       foreach ($_SESSION as $key => $value) {
         $this->$key = $value;
       }
       //$this->usergroup = (!empty($_SESSION['usergroup_status'])) ? array_map('strtolower',$_SESSION['usergroup_status']) : array();
       $this->usergroup = (!empty($_SESSION['usergroup_and_type'])) ? $_SESSION['usergroup_and_type'] : '';
       $this->activeuser = (!empty($_SESSION['activeuser'])) ? true : false;
       $this->isleader = (!empty($_SESSION['usergroup_and_type']) && (in_array($_SESSION['usergroup_and_type'][$group]['typename'],array('styreleder','styremedlem')))) ? true : false;
       $this->issuperuser = (!empty($_SESSION['userrole']) && ($_SESSION['userrole']['rolename'] == 'superadmin')) ? true : false;
       $this->ismainadmin = (!empty($_SESSION['userrole']) && (($_SESSION['userrole']['rolename'] == 'superadmin') || $_SESSION['userrole']['rolename'] == 'hovedadmin')) ? true : false;
       $this->isadmin = (!empty($_SESSION['userrole']) && (($_SESSION['userrole']['rolename'] == 'superadmin') || ($_SESSION['userrole']['rolename'] == 'hovedadmin') || ($_SESSION['userrole']['rolename'] == 'admin'))) ? true : false;
       $this->isgroupadmin = (!empty($_SESSION['userrole']) && (($_SESSION['userrole']['rolename'] == 'superadmin') || ($_SESSION['userrole']['rolename'] == 'hovedadmin') || ($_SESSION['userrole']['rolename'] == 'admin') || ($_SESSION['userrole']['rolename'] == 'gruppeadmin'))) ? true : false;
       $this->alladminaccess = (!empty($_SESSION['userrole']) && (in_array($_SESSION['userrole']['rolename'],array('superadmin','hovedadmin','admin')))) ? true : false;
       $this->shiftleader = $g_user['shiftleader'];
       $this->liquorbar = $g_user['liquorbar'];
       $this->foodcourt = $g_user['foodcourt'];
       $this->beerpub = $g_user['beerpub'];
       $this->workalone = $g_user['workalone'];
     } else {
       $error = displayErrors($get_user);
     }
   }

   function getMessages($core) {
     $stmt = ($this->issuperuser) ? $core->dbh->prepare("SELECT * FROM persdb_user_messages WHERE `read` = 0") : (($this->alladminaccess) ? $core->dbh->prepare("SELECT * FROM persdb_user_messages WHERE `sent_to_group_id` != 0 AND `system_message` = 1 AND `read` = 0") : $core->dbh->prepare("SELECT * FROM persdb_user_messages WHERE `read` = 0 AND user_id = :userid"));
     if ($stmt->execute(array(':userid'=>$this->userid))) {
       if ($stmt->rowCount() > 0) {
         return $numrows = $stmt->rowCount();
       }
     }
   }

   public static function getInstance() {
     if (!isset(self::$instance)) {
       $object = __CLASS__;
       self::$instance = new $object;
     }
     if (!empty(self::$error)) {
       return self::$error;
     }
     return self::$instance;
   } // end getInstance
}

PHP:

I'm not very good with OOP-PHP, so I might have done a lot of stupid stuff here, but it works - however, now I need to be able to add a variable with specific content when constructing - as you can see, I use
$user = new User::getInstance();
to trigger the class in my files - what I would like is to be able to provide something like this:
$user = new User::getInstance('variable content goes here');
And that be available t the __construct part of the class - reason is I'm gonna need to be able to provide specifics to base the access-control on, and this will mainly be page-specific items.

Could anyone have a look at the class, and perhaps tell me what I need to change, and if I need to reconstruct more of my class?

 

I don't think that you should be using Singleton in this case. The definition of Singleton Pattern is:

In your case you are constructing a user object, which will never be the same for 2 different users. I mean if you really have to (I wouldn't recommend it) you can pass those parameters like so:

$user = User::getInstance('admin');

Code (markup):

and then pass the variable from the static variable to the constructor like so:

private function __construct($group) {
....
public static function getInstance($group = null) {
     if (!isset(self::$instance)) {
       $object = __CLASS__;
       self::$instance = new $object($group);
....

Code (markup):

You can clearly see the drawback here is that once you set the group, you won't be able to change it, unless you add the logic to do so in the getInstance() method.

 

I see. Could you maybe recommend a different approach to achieve what I want? I'm getting that a singleton isn't the best way, perhaps you could point me to a better approach that will lend a bit more flexibility to this?

 

I'm a little confused on exactly what you are trying to achieve here. If you can elaborate a more specific use-case, I'd be happy to recommend an ideal solution.

 

If this is a user-auth class, you could just do something much more simple like this:

<?php

class User {

  public $user, $logged_in;

  public function __construct ($group = null) {
    // not sure what $group does, but you can call this to re-authorize a user over and over

    // authenticate user here, get a result from DB

    $this->user = $user_result_from_db;
    $this->logged_in = true;

    // do more things?
  }

}

$user = new User('admin?');

PHP:

Maybe I'm missing something here... feel free to provide more info

 

Well, it's a little complex - the point is a user is the base (of course) - a user can be a part of several groups (workgroups), several time-periods, several roles and several types (roles and types are different states the user is in, based on each group and the system as a whole) - a combination of all of these is what grants a a user the right to view certain settings or change certain values and so forth and so on.
I fixed this temporarily without providing a given variable, I just pulled the information I already had and parsed the arrays it gave back - seems to be working fine for the moment, I'll use that for now, and rethink the class later on

 

I would personally use the User class strictly for holding the data that is/identifies the user. The logic for what is displayed should be handled elsewhere in my opinion.

I'd rather have a bunch of smaller objects/classes than one big confusing one. The more files the better

 

The user is all the data - of course, I could make a new class which extends the user-class, but I don't really see the point? Regardless, it doesn't really do anything about what is actually displayed - it just provides objects for using to discern what is being shown. Maybe that's the same thing, and I'm misunderstanding. The user-class, for instance, provides a true / false for whether or not the user is an admin (any type of admin, there are 4 diffferent ones) and that value can be used other places in the application.

 

I think we're both mis-understanding each other. I still don't know exactly what you were trying to achieve in the first place.

 

Oh, well, the class in question worked fine for the start of the app, but then there was requests for a more fine-tuned discerning of access-rights - hence, before, you would have access to most of the stuff available if you for instance had an Admin-role (roles are site-wide) and then were a board-member for ANY group - this was deemed too loose, and there was a request to tighten it up to just give access to the parts pertaining to the group(s) you had high enough ranks in etc. (The basics - the complete overhaul is a bit more complex than that as well )

I wasn't really trying to achieve much, I was just trying to see if I could avoid having to make too much code elsewhere in the app (which I ended up doing anyway, mostly because it was the simpler thing to do to get this out the door quick.

 

Generally speaking a singleton DOES make sense for tracking the USER -- IF you use setters and getters to isolate the scope of information that 'normal code' shouldn't be able to change.

... but really you've got WAY too much sensitive stuff in the global scope where it could be dicked with -- AND too much information in your sessions which could make sessions slower. You know how too many cookies can make file requests slower? More information that you may or may not be using on every PHP call in your SESSION can have the same type of problems. There's a reason 99.99% of the time the only thing I store in $_SESSION is the ID of the current user, then only pull specific information about said user when I need it.

Said "getters" seeing if the value requested is already stored in the object, if not pull it from the database by that ID.

Your code also has a pretty big nube-ish flub -- you are checking the same values over and over again. Inline evaluations are nice, but don't use them instead of "if" to check things you already checked. Likewise if checking for a bunch of values on the same variable, that's CASE's job... and if you want TRUE and FALSE as the return, you don't have to say "? true : false" -- complete waste of code.

	// I'd assign all of these as false in their object scope declaration
	
	if (!empty($_SESSION['userRole'])) switch ($_SESSION['userRole']) {
		case 'superAdmin':
			$this->isSuperuser = true;
		case 'hovedAdmin':
			$this->isMainAdmin = true;
		case 'admin':
			$this->isAdmin = true;
			$this->allAdminAccess = true;
		case 'gruppeAdmin':
			$this->isGroupAdmin = true;
		break;
	}

Code (markup):

Always keep in mind the drop-through capabilities of a switch/case. Until you say break, it'll pass-through setting them all...

Though with permissions like that, I'd be tempted to use and/or bitwise logic instead of separate variables. Then you could use <> for certain levels of access, as well as "$this->access and permission" for testing. You could then store it far more efficiently in the database that way in up to 32 permissions per variable if stored as a DWORD...

define('SUPER_ADMIN', 0x40000000);
define('MAIN_ADMIN',  0x20000000);
define('ADMIN',       0x10000000);
define('GROUP_ADMIN', 0x08000000);

Code (markup):

for example. "all admin" would then be 0x78000000. Testing for "super admin" would simply read:

if ($user->accessLevel & SUPER_ADMIN) {

Code (markup):

 

^ this guy gets it, but I don't think this was a coding class Good tips for the guy nonetheless

 

@deathshadow - nice list I'm in the process of redoing the whole class, so this is good advice to bring with - I'll have a more in-depth look at it later today. And yes, I know that the SESSION-variable is way overcrowded - unfortunately, I wasn't the one designing the login-bit, and since it's already there, I decided to just grab them as needed. It's gonna be changed as well, but that's not currently a priority.