Registry pattern to get rid of global variables

1 Description

There should be a singleton-registry that allows the retrieval of globals without using the global keyword.
 
Reasons:

• A registry would help us to get rid of the ugly global calls.
• During unit tests, the contents of such a registry could be changed in order to let the tests work with test-doubles.

2 Status

• Funding: Databay AG AG agrees to develop and document that class at no cost for the ILIAS community.
• Development: Feature is to be developed by Databay AG AG

3 Additional Information

• If you want to know more about this feature, its implementation or funding, please contact: Michael Jansen ( mjansen@databay.de ) / Maximilian Becker ( mbecker@databay.de )

4 Discussion

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40

class ilGlobal
{
	private $globals = array();
	private static $instance;
 
	private function __construct()
	{
	}
 
	private function __clone()
	{
	}
 
	public static function getInstance()
	{
		if(self::$instance == NULL)
		{
			self::$instance = new self();
		}
		return self::$instance;
	}
 
	public function __get($name)
	{
		if(array_key_exists($name, $this->globals))
		{
			return $this->globals[$name];
		}
		else
		{
			global $$name;
			return $$name;
		}
	}
 
	public function __set($name, $value)
	{
		$this->globals[$name] = $value;
	}
}

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64

//SUT:
	function getSkillLevelNameById($a_id)
	{
		require_once 'Customizing/global/plugins/Services/UIComponent/UserInterfaceHook/Rewe/classes/class.ilGlobal.php';
		$instance = ilGlobal::getInstance();
		$ilDB = $instance->ilDB;
 
		$ilDB->setLimit(1,0);
 
		$result = $ilDB->query(
			'SELECT title 
			FROM skl_level 
			WHERE id=' . $ilDB->quote($a_id, "integer")
		);
 
		$this->firephp->log($ilDB->db->last_query, "Query in ". __METHOD__ );
 
		if ($ilDB->numRows($result) == 0)
		{
			include_once 'Customizing/global/plugins/Services/UIComponent/UserInterfaceHook/Rewe/exceptions/class.ilRewePluginException.php';
			throw new ilRewePluginException('Method ' . __METHOD__  . ' call failed: No such skill level id. Given: $a_id ' . $a_id);			
		}
 
		while ($row = $ilDB->fetchAssoc($result))
		{
			$skill_level_name = $row['title'];
		}
 
		return $skill_level_name;
	}
 
// Test:
	public function test_GetSkillLevelNameById()
	{
		// Arrange
		require_once 'Services/Database/classes/class.ilDB.php';
		$ildb_stub = $this->getMock('ilDB');
 
		$ildb_stub->expects($this->any())
			->method('query')
			->will($this->returnValue('foo'));
 
		$ildb_stub->expects($this->any())
			->method('numRows')
			->will($this->returnValue(1));
 
		$ildb_stub->expects($this->any())
			->method('fetchAssoc')
			->will($this->onConsecutiveCalls( array('title' => 'Hoddel'), null));
 
		$expected = 'Hoddel';
 
		require_once 'Customizing/global/plugins/Services/UIComponent/UserInterfaceHook/Rewe/classes/class.ilGlobal.php';
		$instance = ilGlobal::getInstance();
		$instance->ilDB = $ildb_stub;
 
		require_once 'Customizing/global/plugins/Services/UIComponent/UserInterfaceHook/Rewe/classes/class.ilReweCompliancePlugin.php';
		$rcp = new ilReweCompliancePlugin();
 
		// Act
		$actual = $rcp->getSkillLevelNameById(2);
 
		$this->assertEquals($actual, $expected);
	}

class ilFoo
{
	private function bar()
	{
		ilGlobal::getInstance()->ilCtrl->getLinkTarget(...);
	}
}

class ilFoo
{
	private function bar()
	{
		ilC::getLinkTarget(...);
	}
}

MB, 25 Jul 2012:

The solution I posted above is an intermediate solution that currently helps me to put critical code under test that I need to immediately report back to me. The implementation of ilGlobal as shown here is not meant to entirely fulfill the requirement that this feature request describes.

I strongly dislike the idea to add more "magic" to ILIAS. As in: I have "ferocious emotions" about it. My personal goal is to eliminate "magic" within the code I am responsible for and encourage others to do the same.
Quite some of the bugs I have to work on come to effect from such ... arcane ... craftsmanship ... which this muggle does not understand.
 
My opinion on context sensitivity: The benefits of unit testing are gone once the systems under test behave differently when they're tested. In the ideal world (which I have "proof of concept" code for :-P ), we can test the exact code that is executed during regular runtime.
 
In regards to the static facades, I have to admit that I do not really see how we can change the concrete class the static facade has to serve up in order to put in test doubles. If you could provide a sample how the sut and test would look like with the facades in place as you see them, it might help me understand.

//SUT:
	function getSkillLevelNameById($a_id)
	{		
		ilD::setLimit(1,0);
 
		$result = ilD::query(
			'SELECT title 
			FROM skl_level 
			WHERE id=' . ilD::quote($a_id, "integer")
		);
 
		$this->firephp->log(ilD::getLastQuery(), "Query in ". __METHOD__ );
 
		if (ilD::numRows($result) == 0)
		{
			include_once 'Customizing/global/plugins/Services/UIComponent/UserInterfaceHook/Rewe/exceptions/class.ilRewePluginException.php';
			throw new ilRewePluginException('Method ' . __METHOD__  . ' call failed: No such skill level id. Given: $a_id ' . $a_id);			
		}
 
		while ($row = ilD::fetchAssoc($result))
		{
			$skill_level_name = $row['title'];
		}
 
		return $skill_level_name;
	}
 
// Test:
	public function test_GetSkillLevelNameById()
	{
		// not sure how this exactly works...
		require_once 'Services/Database/classes/class.ilDB.php';
		$ildb_stub = $this->getMock('ilDB');
 
		$ildb_stub->expects($this->any())
			->method('query')
			->will($this->returnValue('foo'));
 
		$ildb_stub->expects($this->any())
			->method('numRows')
			->will($this->returnValue(1));
 
		$ildb_stub->expects($this->any())
			->method('fetchAssoc')
			->will($this->onConsecutiveCalls( array('title' => 'Hoddel'), null));
 
		$expected = 'Hoddel';
 
		ilD::setMock($ildb_stub);
 
		require_once 'Customizing/global/plugins/Services/UIComponent/UserInterfaceHook/Rewe/classes/class.ilReweCompliancePlugin.php';
		$rcp = new ilReweCompliancePlugin();
 
		// Act
		$actual = $rcp->getSkillLevelNameById(2);
 
		$this->assertEquals($actual, $expected);
 
		// I guess we should reset to the normal behaviour here, the "$instance->ilDB = $ildb_stub;" above looks quite harsh to me
		ilD::removeMock();
	}

MB, 26 Jul 2012:
I have no doubt that every single piece of "magic" was introduced with the authors having best intentions. Still, exactly the behaviour described is hiding logic from the surface and such code is what has the potential to contradict the intentions of a programmer who is not aware of these hidden ongoings. Such behaviour is a classic side-effect: You call out to the system in order to get or achieve something and the code does almost what you expect it to do - but something different as well and without telling. This makes the system opaque and fragile - and so its behaviour becomes to the other auther nonsense for his work and the system behaves unpredictable. It's a matter of perspective. You, as well as the whole crew of battle-proven core developers, know perfectly how to avoid these puddles. This is a problem for new developers as it flattens their learning curve considerably.
 
Besides that, I would always prefer a solution in which the test author controls the preparation of a system under test before its examination and so I would not want to save them from injecting fake objects on their own.
 
Finally, I can see how you think of the facade and it looks feasible at first. I think this approach might work if a possible mock object wouldn't have to implement all methods of the original object. The mock-objects from phpunit do that, still this doesn't need to be the case for all mocking-solutions that may cross our ways in the future.
 
There are disadvantages to it, though:

• The number of changes is less, if we can populate a global in one place, rather than changing code all over the place.
• While using the facade as static call in the code is better than using a global, static calls are hard to test as well. So it may have future merit to leave the question where the object comes from in one place of the method until we have it refactored to something that follows good object oriented principles. If we have that, the code would actually use the object in the methods "main-body" as it is used now. If we moved to the static calls first, we would have to change that back, again with lots of changes all over the place.

Alex 26 Jul 2012:
 
Max, it is a little but difficult to follow your arguments, cause you do not refer to the code.
 
"Still, exactly the behaviour described is hiding logic from the surface"
 
What logic is hidden?
 
"Besides that, I would always prefer a solution in which the test author controls the preparation of a system under test before its examination"
 
Why is this not the case for the second alternative?
 
"I think this approach might work if a possible mock object wouldn't have to implement all methods of the original object. The mock-objects from phpunit do that, still this doesn't need to be the case for all mocking-solutions that may cross our ways in the future."
 
So your preference is that they must implement all methods or not? It sounds more like "or not" alternative is your preference in which case the facade approach "might work". However I cannot see in general, why these two alternatives differ in this respective in any way. Why is that?
 
"The number of changes is less, if we can populate a global in one place, rather than changing code all over the place."
 
This is true, if we would go for migratring all code and totally abandon the old code. I didn't think that we would do this for both alternatives. Means: the gobal will still be with us a while, but new code could be written in a leaner way. Lean code was also my major objective, since leaner code is easier to read. Code like "$instance = ilGlobal::getInstance; $ilDB = $instance->ilDB" is mainly overhead that produces clutter and distracts developers frm the real application logic.
 
"While using the facade as static call in the code is better than using a global, static calls are hard to test as well."
 
What makes static calls harder to test? And aren't we mainly testing functions that just use these few classes, not the classes themselves? Or are all functions that include any static call harder to test?
 
So it may have future merit to leave the question where the object comes from in one place of the method until we have it refactored to something that follows good object oriented principles. If we have that, the code would actually use the object in the methods "main-body" as it is used now. If we moved to the static calls first, we would have to change that back, again with lots of changes all over the place.
You lost me here. What is definitely true is, that it would require more changes, if we move completely to the static calls. But we would end up with leaner code.

MB, 26 Jul 2012:
I work through your post by paragraph:
 
What logic is hidden?
The first paragraph was about the general concept of "magic" in the code. The behaviour you describe that I mentioned is from your first post, in which you write: ilContextUnitTest would trigger any magic?
For the purpose of this feature I have understood, that we both do not want the system to act unpredictable and I have the feeling the difference we have is about the definition of it. I'd say we should postpone the finding of this definition to a personal meeting (with beer).
 
Why is this not the case for the second alternative?
If a ilContextUnitTest would trigger magic implicitly, the tester would not have to do it explicitly.
 
So your preference is that they must implement all methods or not?...Why is that?
Not all methods. I favor a solution in which I can pass in a small solution because I may have to be forced to construct it. If you see the first code I sent, in lines 39 - 50 I prepare an otherwise empty object with the methods and results that the tests needs. Enough work in that.
The mock objects of phpunit ship with empty methods, so there it doesn't make a difference. But if I had to build a full interface on the fake so the facade accepts it in place of the real object, this work would be considerably more.
 
What makes static calls harder to test?
The static method can of course be tested, it's their callers that get us into trouble: Static Methods are Death to Testability
If I have a method that calls a method on another class, then this other class must not be subject of my test. So I take the class and method out of all context and see if the code in it works. I put the sorry few lines of code out and plug in everything it barely needs to execute: I create a mock object of this second class (here: ilDB) and set it up to cooperate - I expect the call to query, could even check if this is with the right params (which is not shown above), then return something (so the method under test doesn't notice) and see if the code I test works with that. The other class will be tested separately. A static call is bound to a concrete class and so I cannot inject something that takes its place (easily).
 
What is definitely true is, that it would require more changes, if we move completely to the static calls. But we would end up with leaner code.
True, but we had the statics all over the place which I also want to get rid of in the long term. The desired way to get foreign instances into the code are constructors and/or setter methods.

Alex, 26 Jul 2012:
 
Ok, I was confused that you refered to my first post after the second one. But I finally understand the major flaw in the static approach. Thanks for your explanation and the the link to the article. I was not aware of the problems with respect to testability. So this would definitely not be an option.
 
Still somehow...
 
$instance = ilGlobal::getInstance();
$instance->ilDB = $ildb_stub;
 
...reads to me a little bit like:
 
$_GLOBALS["ilDB"] = $ildb_stub;
 
The test changes something in a global-like state, which would affect another test in the same run? Or would there be some automatic reset? Is there a better way to do this? I read about dependency injection, but I would not like to pass database objects within constructors of all application classes... :-)

MB, 29 Jul 2012:
No matter how long I think about it, I come to only one conclusion: Unfortunately, you are right ;-)
It is simpler to replace an eventual current global with an own one. I will bring this matter up in the office and find out if Agent H. and Agent J. can support the idea with some arguments. (If they haven't, I'll close the page then.)

MB, 03 Aug 2012:
After talking to my colleagues, we come to a statement:
 
We find the use of globals all over the place in ILIAS annoying and contradicting good design and coding habits.
The implementation of a singleton registry would be a minor enhancement, still we admit that a singleton registry with static methods is not enough of an improvement in order to keep this feature request up.

AK 10 Aug 2013: Today I was thinking again about the registry and maybe there are some points that we could add:

• Having some kind of hard-coded "white list" of objects that can be registered ("DB", "Ctrl", "...") in this registry. This would allow us to have a real restriction on what objects act in a "global" way. It would not be possible to register a "Foo" object which is not in this list, this would throw an exception. We would have one central place that tells every developer what is available through the registry.
• Get rid of the else { global $$name; return $$name; } part above and throw in exception in this case. Call it something like ilRegistry or ilAppReg and not ilGlobal, just to make it even clearer, that this is not trying act like "global".
• Would it be helpful, if the registry would "have knowledge" on objects being testing doubles? So instead of registering them like "the real objects" could something like $reg->setDouble("DB", $mydb); and $reg->clearDoubles() be of any value?

MB, 12 Aug 2013: I am really happy you thought about the request again and I really appreciate the energy you put into the matter.
 
Whitelist: If I may be so bold and sketch a future design iteration. In such a scenario, the use of a globals object should be ending. So yes, this is absolutely a way to go, but this is a future step. Here's why: If I think about this today, my first idea is to remove this restriction or not implement this restriction on the $ILIAS-object, so we can pass the globals through our constructors, disguising the whole globals mechanism within one object and have the whole toolbox with us all the time. But this would only mean, that we circumvent the restriction of the globals object, hide the fact that we're still actually kind of global and open up the next messy place we need to take care about at a later time. The only way to get around the existing need to randomly access global objects is that we instantiate our objects with just the necessary objects in the constructor (or the "construction sequence"). Such can be done nicely if we could get our objects from a central factory that knows what's needed to pass out fully functional objects. Once we have factories in place that handle this for us, the use of globals can and should be ending.
 
$$name: Absolutely. The less $$'s und $->$'s we have, the better for our sanity and static code analysis tools. :-)
 
Knowledge: In order to test the actual system, we should avoid "test specific code". The more natural the system behaves, the better. If we inject a test double there and "noone really cares", then this is good abstraction.

JF 19 Aug 2013: We think that a central object that returns commonly needed objects could be useful. A whitelist would be useful, too, and could be implemented by using non-generic static getter/setters (getCurrentUser(), getDB(),...). Object that are minor parts of bigger modules (e.g. ilTabs) could not be returned directly but by a higher object (e.g. getUI()).
 
But - we are unsure whether the added-value justifies the effort that is needed to refactor the code.
 
We will keep on the discussion and put it on the 4.5 agenda.

MB 23 Oct 2013: We currently explore the possibilities to implement an autoloader and a dependency injection container in a project. This involves a mechanism that allows for an incremental implementation (as we did not get the funding to comprehensively overhaul all ILIAS).
 
Current goals (within fulfilling the customers requirements) are:

• Getting rid of globals without needing to refactor all ILIAS code using dependency injection. Using a DIC for setter-injections (here: Pimple) in ILIAS in favour of a registry object to support IOC.
• Figuring out an ILIAS compatible and feasible namespace-scheme to support the Framework Interop Groups'  PSR-0 / PSR-4 standards to allow for cool(tm) autoloading and creation of classes.

AK 2 Apr 2014: I support the DIC idea and also think PSR-4 would be something we should try. See also Colins FW request to use DIC+reflection.

5 Implementation