Circular reference of classes

[Firemankurt]

I have a core class that does the majority of the heavy lifting for my framework.  

I also use an external class "access" to check for user access to resources.

While trying to streamline things a bit I noticed that when I create an "access"

object outside of my "core" class that I have to declare my "access" object as global

anytime I want to use it inside the "core" class.

 

Example:

class core()

{

public somefunction()

{

global $Access;

//... Etc ...

}

}

 

$Access = new access();

 

$Core = new core();

 

$Core->somefunction();

 

I want to do:

 

class core()

{

public somefunction()

{

//  $this->Access now available here w/o declare global

  //... Etc ...

}

public initAccess()

{

  $this->Access = new access();

//... Etc ...

}

}

 

 

The Problem is that "access" class uses the DB connection and query function inside "core".

I am worried about circular references.  Any gurus have advice on such an issue or am I worried about nothing?

 

Here is a snippet of the actual code pared down to minimum:

<?php
class myCore
{
	private $_Ini=array(); 	// Main Ini Info Array
	public $_DB='';			// DB Connection
	private $_ACCESS=false;	// Hold Access Class Instance
	
	public function __construct() {
		$this->_Ini 			= parse_ini_file(ABSPATH.DS."config.php", true);
		$this->connect();
		$this->initAccess();
	}// End Constuct

	public function connect()
	{
		if(!defined(HOST))
		{
			$dbauth = $this->_Ini['database'];
			define ('HOST',$dbauth['HOST']);
			define ('DB_USER',$dbauth['DB_USER']);
			define ('DBPASS',$dbauth['DBPASS']);
			define ('DB',$dbauth['DB']);
		}
		$this->_DB = @new mysqli(HOST, DB_USER, DBPASS,DB);

		/* check connection */
		if ($this->_DB->connect_error)
			trigger_error('Database connection failed in '.__FILE__.' at '.__LINE__.':'.$this->_DB->connect_error, E_USER_ERROR);
	} //end connect
	
	// Central Query Function
	public function DBquery($query,$Location) 
	{
		$result = $this->_DB->query($query) or
		trigger_error('<strong>Query Error:</strong> '.$this->_DB->error.
			'<br/><strong>Query Location:</strong> '.$Location.
			'<br/><strong>Query:</strong> '.$query, E_USER_ERROR);
		return $result;
	}// End DBquery()
	
	// Initiate Access class
	public function initAccess() 
	{
		$this->_ACCESS = new Access($_SESSION['UsrNum'],$this);
	}// End initAccess()
	
	/*
	 * Bunch of other Functions...
	 */
}



class Access
{
	/**
	* Checks user access for resources
	*/ 
    private $user;
    private $_CORE;

	public function __construct($user,&$CORE_Class)
	{
		$this->_CORE = $CORE_Class;
		$this->user = $user;
	} // End __construct
	
	public function getRole()
	{
		$result = $this->_CORE->DBquery('SELECT `group` FROM `user_role` WHERE `user`='.$this->user,__FILE__.' at '.__LINE__);
		while($row=$result->fetch_array(MYSQLI_ASSOC)) $role[]= $row['group'];
		return $role;
	} // End getRole
	
	/*
	 * Bunch of other Functions...
	 */
}

$MYcore = new myCore();
$MYcore->_ACCESS->getRole();
?>

 

[rbrown]

You really shouldn''t be using global variables in your classes.

 

read this link:

http://forums.phpfreaks.com/topic/205920-why-should-you-not-use-global-variable/

 

 

I use a singleton pattern for db connections. That way you only have one instance running instead of a bunch of them. On a smaller sites won't matter, but on a larger site, having a bunch of instances open for each user will quickly start choking the server.

 

When you have a state-dependant class you should not use a singleton pattern.

 

The rule is... If you can use a variable to hold an instance of the class and reuse the same variable for a different class on the same page without breaking anything then use a singleton pattern.

[Firemankurt]

I read a few articles on patterns and singleton patterns.  My head is swimming a bit in these abstract concepts but I think I am starting to understand them.  I agree that I shouldn't be using global variables in my classes and that is part of what lead me to want to make these changes.

 

I also noticed an error in my code snippet:

 

private $_ACCESS=false; // Hold Access Class Instance

 

Should have been

 

public $_ACCESS=false; // Hold Access Class Instance

 

So with my limited understanding of the singleton pattern I am still not sure I understand how this helps my situation.

I have changed:  public function connect() to     private function connect() to prevent creation of multiple DB connections.

My "access" class could be a singleton and my "core" could be a singleton but how would I integrate these?

Sorry but I just need a little more detail.

[ignace]

Core is pretty abstract for a class name. And unless you are really into God objects (yes no christians among programmers, we use the name in vain), it would be better if you broke it down further into smaller re-usable components. (see EDIT2 below)

 

Otherwise simply pass the Access class through the constructor, or unless it can operate without it, which I doubt, through a setter.

 

class Core {
  private $access;
  
  public function __construct(Access $control) {
    $this->access = $control;
  }
  
  protected function getAccess() {
    return $this->access;
  }
}

It's also a good practice not to use public variables on a class since that way you lose all control. Protected should be avoided as well if possible (what public is to the outside 'world' is protected to the child classes). If child classes need it make it available through a getter as shown.

 

Singletons should be avoided as well, it's the same as the global keyword. If a class needs another object to operate, simply pass it through the constructor.

 

EDIT2:

For example, the $_Ini could be a Config object that was created by a Factory that could read from different possible sources .ini .xml .json .. Take a look at:

https://github.com/zendframework/zf2/tree/master/library/Zend/Config

 

Your hardcoded mysqli inside Core could be passed through the constructor. Though I would advice to use PDO:

 

class Core {
  public function __construct(PDO $pdo) {
    $this->driver = $pdo;
  }
}

PDO is a data access layer, a database abstraction layer is available at:

https://github.com/zendframework/zf2/tree/master/library/Zend/Db

 

The difference is that Zend/Db allows you to write abstract queries:

$select = new Select('users');
$select->where(array('username' => 'ignace'));

You can use these libraries instead of writing them yourself. If you look around it's quite possible some folks have already done what you want to do and you can simply use their effort and make it your own. Edited December 29, 2013 by ignace

[Firemankurt]

My Project is called Ladder CMS.

 

"Core" is actually called "ladderCore" but I did not think that was really significant to my issue here so I renamed it for this post.  

 

"ladderCore" and many of it's properties needs to be globally accessable.

 

My issue boils down to the fact that "ladderCore" handles orchestration of DB connection, queries, Log-in, Error collection, module loading, Output variables into template, module access, etc.

 

"access" is another class that is used in just about every module and "ladderCore" to decide weather users have acces to that module and it's resources.

 

So it flows kinda like this:

 

• index.php loads:
  • Ini.php (finds install location, sets some constants, parses config for Ladder CMS installation)
    it loads:
    • LadderCore.php ( main class created as  $LCMS by Ini.php)
    • class.access.php ( access checker class dependant on  $LCMS->DBquery() and $LCMS->_ERROR  )
    • class.DBSession.php ( Database driven Session handler dependant on  $LCMS->DBquery() and $LCMS->_ERROR )
• protect.php ( Handles login forms, cookies, sessions )
  • protect.php creates  access checker class as  $LCMSAccess and injects values based on log in. 

index.php then uses $LCMSAccess, $LCMS->FilterMods() , $LCMS->_MODTOINCLUDE, $LCMS->_TEMPLATE and other ladderCore or module methods to create content.

 

I know it is not perfect.  I have been consolidating 8-10 projects into this framework over the past 8 years and I am still refining things.

 

One of the problems I am trying to solve is the global keyword used to grant access to the methods of $LCMSAccess in just about every file and many classes.

 

I would like to put it into a class property like $LCMS->Access since $LCMS class is available just about everywhere.

 

Since $LCMS->Access contains the 'access' class and 'access' class contains  $LCMS, am I creating a memory leak here or other catastrophic situation?

 

Have I been staring at this so long that I am loosing my mind or will this work just fine?

[Firemankurt]

Here is a little test of the structure that represents a simplistic model of what I am concerned about:

<?php
class LCMScore {
    public $Access = null;
    
    public function __construct(){}
    public function CreateAccess($var1,$var2)
    {
		 $this->Access = new myAccess($this , $var1, $var2);
	}

    public function addit($var1,$var2)
    {
		return $var1+$var2;
	}
}


class myAccess 
{
    private $var1 = null;
    private $var2 = null;
    private $LCMS = null;
     
    public function __construct(&$LCMS, $var1,$var2)
    {
		$this->LCMS = $LCMS;
		$this->var1 = $var1;
		$this->var2 = $var2;
	}
  
    public function doAdd()
    {
		return $this->LCMS->addit($this->var1,$this->var2);
	}
}

$a = new LCMScore();
$a->CreateAccess(4,5);
echo $a->Access->doAdd().'<br />';
sleep ( 10);
echo memory_get_peak_usage (true);
?>
 

I set the sleep ( 10) to sleep ( 1) and I always get a result of 262144 from memory_get_peak_usage (true).

Does this mean I am in the clear?

Edited December 30, 2013 by Firemankurt

[ignace]

I am not sure what it is you expect from us. But, I would use a Dependency Injection container, because:

 

"ladderCore" and many of it's properties needs to be globally accessable.

Nothing has to be globally accessible. It's bad practice for a class to be globally accessible. You solve this by using a DI container:

 

$pimple = new Pimple():
$pimple['ini'] = function($c) {
  // read and return configuration values
};

$pimple['db'] = function($c) {
  $config = $c['ini'];
  // create database
};

$pimple['access'] = function($c) {
  return new Access($c['db']);
};

$pimple['some_module'] = function($c) {
  return new MyModule($c['access']);
};

// ...

As you can see from this container I can pass any required information to whichever class that needs it without having to be globally accessible. Edited December 31, 2013 by ignace

[trq]

My issue boils down to the fact that "ladderCore" handles orchestration of DB connection, queries, Log-in, Error collection, module loading, Output variables into template, module access, etc.

 

That is indeed one of your issues. Classes should be responsible for one thing, and one thing only. Your *core* class does far too much.

 

As for your issue around globals. I'll just say I completely agree with ignace.

[Firemankurt]

Thank you both for your patients and responses.  Being a lone developer of a large project has serious limitations and being a member here sure helps.

 

I think DI containers are the next concept for me to grasp and use to solve my problem here.

 

It is tough finding time to learn all these concepts and harder to learn what has changed between PHP versions.  I have tried to set some time aside from actual coding to educate myself but there is always something new to cram into my little CPU and memory.

 

This is what I love about coding.... Always something new to learn.

[ignace]

Since $LCMS->Access contains the 'access' class and 'access' class contains $LCMS, am I creating a memory leak here or other catastrophic situation?

No bi-directional relationships are fine. Though your example is awkward at best, why not simply call addit() on LCMSCore instead of creating a separate class to do that for you?

 

Also, separation of concern. Why does LCMSCore create the Access class? What if their will be multiple ways to build the Access class in the future? Will you add all of these methods on LCMSCore?

 

And what if you need to do more then simply add? multiply? divide? substract?

 

namespace Math;

interface Operation {
  public function execute();
}

class Operand implements Operation {
  private $value;
  
  public function __construct($op) {
    $this->value = $op;
  }
  
  public function execute() {
    return $this->value;
  }
}

abstract class AbstractOperation implements Operation {
  protected $op1;
  protected $op2;
  
  public function __construct(Operation $op1, Operation $op2) {
    $this->op1 = $op1;
    $this->op2 = $op2;
  }
}

class Add extends AbstractOperation {
  public function execute() {
    return $this->op1->execute() + $this->op2->execute();
  }
}

echo (new Add(new Operand(1), new Add(new Operand(2), new Add(new Operand(1), new Operand(2)))))->execute(); // 6

If you need to add multiplication:

 

class Multiply extends AbstractOperation {
  public function execute() {
    return $this->op1->execute() * $this->op2->execute();
  }
}

 

I set the sleep ( 10) to sleep ( 1) and I always get a result of 262144 from memory_get_peak_usage (true).

Does this mean I am in the clear?

No. That's not how you locate a memory leak. You have to understand how it works, and how it 'leaks'. Search for 'php find memory leak' and you will find good resources to tools that help you find memory leaking code.

Edited December 31, 2013 by ignace