[feature/request-class] Refactored request class and wrapper functions.

The request class
- now makes use of the new type cast helper (dependency injection)
- has no static methods anymore.
- now has a constructor argument to leave super globals turned on

Brought back the set_var function in functions.php. It is now a wrapper
around the type cast helper. It creates an instance on the fly.

The request_var wrapper function now has an optional last argument to
inject the request class instance, rather than abusing the $var_name.

PHPBB3-9716

phpBB/includes/functions.php

@@ -18,6 +18,13 @@ if (!defined('IN_PHPBB'))
 
 // Common global functions
 
+function set_var(&$result, $var, $type, $multibyte = false)
+{
+	// no need for dependency injection here, if you have the object, call the method yourself!
+	$type_cast_helper = new phpbb_type_cast_helper();
+	$type_cast_helper->set_var($result, $var, $type, $multibyte);
+}
+
 /**
 * Wrapper function of phpbb_request::variable which exists for backwards compatability.
 * See {@link phpbb_request_interface::variable phpbb_request_interface::variable} for
@@ -40,30 +47,30 @@ if (!defined('IN_PHPBB'))
 * @return	mixed	The value of $_REQUEST[$var_name] run through {@link set_var set_var} to ensure that the type is the
 * 					the same as that of $default. If the variable is not set $default is returned.
 */
-function request_var($var_name, $default, $multibyte = false, $cookie = false)
+function request_var($var_name, $default, $multibyte = false, $cookie = false, phpbb_request_interface $request = null)
 {
 	// This is all just an ugly hack to add "Dependency Injection" to a function
 	// the only real code is the function call which maps this function to a method.
-	static $request = null;
+	static $static_request = null;
 
-	if ($var_name instanceof phpbb_request_interface)
+	if ($request instanceof phpbb_request_interface)
 	{
-		$request = $var_name;
+		$static_request = $request;
-		return;
+
+		if (empty($var_name))
+		{
+			return;
+		}
 	}
 
+	$tmp_request = $static_request;
+
 	// no request class set, create a temporary one ourselves to keep backwards compatability
-	if ($request === null)
+	if ($tmp_request === null)
 	{
-		$tmp_request = new phpbb_request();
+		// false param: enable super globals, so the created request class does not
-		// enable super globals, so the magically created request class does not
 		// make super globals inaccessible everywhere outside this function.
-		$tmp_request->enable_super_globals();
+		$tmp_request = new phpbb_request(new phpbb_type_cast_helper(), false);
-	}
-	else
-	{
-		// otherwise use the static injected instance
-		$tmp_request = $request;
 	}
 
 	return $tmp_request->variable($var_name, $default, $multibyte, ($cookie) ? phpbb_request_interface::COOKIE : phpbb_request_interface::REQUEST);

phpBB/includes/request/request.php

@@ -28,7 +28,7 @@ class phpbb_request implements phpbb_request_interface
 	/**
 	* @var	array	The names of super global variables that this class should protect if super globals are disabled.
 	*/
-	protected static $super_globals = array(
+	protected $super_globals = array(
 		phpbb_request_interface::POST => '_POST',
 		phpbb_request_interface::GET => '_GET',
 		phpbb_request_interface::REQUEST => '_REQUEST',
@@ -51,26 +51,26 @@ class phpbb_request implements phpbb_request_interface
 	protected $input;
 
 	/**
-	* @var	string	Whether slashes need to be stripped from input
+	* @var	phpbb_type_cast_helper_interface	An instance of a type cast helper providing convenience methods for type conversions.
 	*/
-	protected $strip;
+	protected $type_cast_helper;
 
 	/**
 	* Initialises the request class, that means it stores all input data in {@link $input input}
 	* and then calls {@link phpbb_deactivated_super_global phpbb_deactivated_super_global}
 	*/
-	public function __construct()
+	public function __construct(phpbb_type_cast_helper_interface $type_cast_helper = null, $disable_super_globals = true)
 	{
-		if (version_compare(PHP_VERSION, '6.0.0-dev', '>='))
+		if ($type_cast_helper)
 		{
-			$this->strip = false;
+			$this->type_cast_helper = $type_cast_helper;
 		}
 		else
 		{
-			$this->strip = (@get_magic_quotes_gpc()) ? true : false;
+			$this->type_cast_helper = new phpbb_type_cast_helper();
 		}
 
-		foreach (self::$super_globals as $const => $super_global)
+		foreach ($this->super_globals as $const => $super_global)
 		{
 			$this->input[$const] = isset($GLOBALS[$super_global]) ? $GLOBALS[$super_global] : array();
 		}
@@ -79,7 +79,10 @@ class phpbb_request implements phpbb_request_interface
 		$this->original_request = $this->input[phpbb_request_interface::REQUEST];
 		$this->input[phpbb_request_interface::REQUEST] = $this->input[phpbb_request_interface::POST] + $this->input[phpbb_request_interface::GET];
 
-		$this->disable_super_globals();
+		if ($disable_super_globals)
+		{
+			$this->disable_super_globals();
+		}
 	}
 
 	/**
@@ -100,7 +103,7 @@ class phpbb_request implements phpbb_request_interface
 	{
 		if (!$this->super_globals_disabled)
 		{
-			foreach (self::$super_globals as $const => $super_global)
+			foreach ($this->super_globals as $const => $super_global)
 			{
 				unset($GLOBALS[$super_global]);
 				$GLOBALS[$super_global] = new phpbb_deactivated_super_global($this, $super_global, $const);
@@ -118,7 +121,7 @@ class phpbb_request implements phpbb_request_interface
 	{
 		if ($this->super_globals_disabled)
 		{
-			foreach (self::$super_globals as $const => $super_global)
+			foreach ($this->super_globals as $const => $super_global)
 			{
 				$GLOBALS[$super_global] = $this->input[$const];
 			}
@@ -129,34 +132,6 @@ class phpbb_request implements phpbb_request_interface
 		}
 	}
 
-	/**
-	* Recursively applies addslashes to a variable.
-	*
-	* @param	mixed	&$var	Variable passed by reference to which slashes will be added.
-	*/
-	public static function addslashes_recursively(&$var)
-	{
-		if (is_string($var))
-		{
-			$var = addslashes($var);
-		}
-		else if (is_array($var))
-		{
-			$var_copy = $var;
-			$var = array();
-			foreach ($var_copy as $key => $value)
-			{
-				if (is_string($key))
-				{
-					$key = addslashes($key);
-				}
-				$var[$key] = $value;
-
-				self::addslashes_recursively($var[$key]);
-			}
-		}
-	}
-
 	/**
 	* This function allows overwriting or setting a value in one of the super global arrays.
 	*
@@ -172,15 +147,12 @@ class phpbb_request implements phpbb_request_interface
 	*/
 	public function overwrite($var_name, $value, $super_global = phpbb_request_interface::REQUEST)
 	{
-		if (!isset(self::$super_globals[$super_global]))
+		if (!isset($this->super_globals[$super_global]))
 		{
 			return;
 		}
 
-		if ($this->strip)
+		$this->type_cast_helper->add_magic_quotes($value);
-		{
-			self::addslashes_recursively($value);
-		}
 
 		// setting to null means unsetting
 		if ($value === null)
@@ -188,114 +160,22 @@ class phpbb_request implements phpbb_request_interface
 			unset($this->input[$super_global][$var_name]);
 			if (!$this->super_globals_disabled())
 			{
-				unset($GLOBALS[self::$super_globals[$super_global]][$var_name]);
+				unset($GLOBALS[$this->super_globals[$super_global]][$var_name]);
 			}
 		}
 		else
 		{
 			$this->input[$super_global][$var_name] = $value;
-			if (!self::super_globals_disabled())
+			if (!$this->super_globals_disabled())
 			{
-				$GLOBALS[self::$super_globals[$super_global]][$var_name] = $value;
+				$GLOBALS[$this->super_globals[$super_global]][$var_name] = $value;
 			}
 		}
 
-		if (!self::super_globals_disabled())
+		if (!$this->super_globals_disabled())
 		{
-			unset($GLOBALS[self::$super_globals[$super_global]][$var_name]);
+			unset($GLOBALS[$this->super_globals[$super_global]][$var_name]);
-			$GLOBALS[self::$super_globals[$super_global]][$var_name] = $value;
+			$GLOBALS[$this->super_globals[$super_global]][$var_name] = $value;
-		}
-	}
-
-	/**
-	* Set variable $result to a particular type.
-	*
-	* @param mixed	&$result	The variable to fill
-	* @param mixed	$var		The contents to fill with
-	* @param mixed	$type		The variable type. Will be used with {@link settype()}
-	* @param bool	$multibyte	Indicates whether string values may contain UTF-8 characters.
-	* 							Default is false, causing all bytes outside the ASCII range (0-127) to be replaced with question marks.
-	*/
-	public function set_var(&$result, $var, $type, $multibyte = false)
-	{
-		settype($var, $type);
-		$result = $var;
-
-		if ($type == 'string')
-		{
-			$result = trim(htmlspecialchars(str_replace(array("\r\n", "\r", "\0"), array("\n", "\n", ''), $result), ENT_COMPAT, 'UTF-8'));
-
-			if (!empty($result))
-			{
-				// Make sure multibyte characters are wellformed
-				if ($multibyte)
-				{
-					if (!preg_match('/^./u', $result))
-					{
-						$result = '';
-					}
-				}
-				else
-				{
-					// no multibyte, allow only ASCII (0-127)
-					$result = preg_replace('/[\x80-\xFF]/', '?', $result);
-				}
-			}
-
-			$result = ($this->strip) ? stripslashes($result) : $result;
-		}
-	}
-
-	/**
-	* Recursively sets a variable to a given type using {@link set_var set_var}
-	* This function is only used from within {@link phpbb_request::variable phpbb_request::variable}.
-	*
-	* @param	string	$var		The value which shall be sanitised (passed by reference).
-	* @param	mixed	$default	Specifies the type $var shall have.
-	* 								If it is an array and $var is not one, then an empty array is returned.
-	* 								Otherwise var is cast to the same type, and if $default is an array all
-	* 								keys and values are cast recursively using this function too.
-	* @param	bool	$multibyte	Indicates whether string values may contain UTF-8 characters.
-	* 								Default is false, causing all bytes outside the ASCII range (0-127) to
-	* 								be replaced with question marks.
-	*/
-	protected function recursive_set_var(&$var, $default, $multibyte)
-	{
-		if (is_array($var) !== is_array($default))
-		{
-			$var = (is_array($default)) ? array() : $default;
-			return;
-		}
-
-		if (!is_array($default))
-		{
-			$type = gettype($default);
-			$this->set_var($var, $var, $type, $multibyte);
-		}
-		else
-		{
-			// make sure there is at least one key/value pair to use get the
-			// types from
-			if (empty($default))
-			{
-				$var = array();
-				return;
-			}
-
-			list($default_key, $default_value) = each($default);
-			$value_type = gettype($default_value);
-			$key_type = gettype($default_key);
-
-			$_var = $var;
-			$var = array();
-
-			foreach ($_var as $k => $v)
-			{
-				$this->set_var($k, $k, $key_type, $multibyte, $multibyte);
-
-				$this->recursive_set_var($v, $default_value, $multibyte);
-				$this->set_var($var[$k], $v, $value_type, $multibyte);
-			}
 		}
 	}
 
@@ -356,7 +236,7 @@ class phpbb_request implements phpbb_request_interface
 			}
 		}
 
-		self::recursive_set_var($var, $default, $multibyte);
+		$this->type_cast_helper->recursive_set_var($var, $default, $multibyte);
 
 		return $var;
 	}

tests/request/all_tests.php

@@ -15,6 +15,7 @@ if (!defined('PHPUnit_MAIN_METHOD'))
 require_once 'test_framework/framework.php';
 require_once 'PHPUnit/TextUI/TestRunner.php';
 
+require_once 'request/type_cast_helper.php';
 require_once 'request/deactivated_super_global.php';
 require_once 'request/request.php';
 require_once 'request/request_var.php';
@@ -30,9 +31,10 @@ class phpbb_request_all_tests
 	{
 		$suite = new PHPUnit_Framework_TestSuite('phpBB Request Parameter Handling');
 
-		$suite->addTestSuite('phpbb_request_deactivated_super_global_test');
+		$suite->addTestSuite('phpbb_type_cast_helper_test');
+		$suite->addTestSuite('phpbb_deactivated_super_global_test');
 		$suite->addTestSuite('phpbb_request_test');
-		$suite->addTestSuite('phpbb_request_request_var_test');
+		$suite->addTestSuite('phpbb_request_var_test');
 
 		return $suite;
 	}

tests/request/deactivated_super_global.php

@@ -11,12 +11,12 @@
 require_once 'test_framework/framework.php';
 require_once '../phpBB/includes/request/deactivated_super_global.php';
 
-class phpbb_request_deactivated_super_global_test extends phpbb_test_case
+class phpbb_deactivated_super_global_test extends phpbb_test_case
 {
 	/**
 	* Checks that on write access the correct error is thrown
 	*/
-	public function test_write_results_in_error()
+	public function test_write_triggers_error()
 	{
 		$this->setExpectedTriggerError(E_USER_ERROR);
 		$obj = new phpbb_deactivated_super_global($this->getMock('phpbb_request_interface'), 'obj', phpbb_request_interface::POST);

tests/request/request.php

@@ -9,6 +9,8 @@
 */
 
 require_once 'test_framework/framework.php';
+require_once '../phpBB/includes/request/type_cast_helper_interface.php';
+require_once '../phpBB/includes/request/type_cast_helper.php';
 require_once '../phpBB/includes/request/request_interface.php';
 require_once '../phpBB/includes/request/deactivated_super_global.php';
 require_once '../phpBB/includes/request/request.php';
@@ -62,16 +64,6 @@ class phpbb_request_test extends phpbb_test_case
 		$this->assertFalse($this->request->is_set_post('unset'));
 	}
 
-	public function test_addslashes_recursively()
-	{
-		$data = array('some"string' => array('that"' => 'really"', 'needs"' => '"escaping'));
-		$expected = array('some\\"string' => array('that\\"' => 'really\\"', 'needs\\"' => '\\"escaping'));
-
-		phpbb_request::addslashes_recursively($data);
-
-		$this->assertEquals($expected, $data);
-	}
-
 	public function test_variable_names()
 	{
 		$expected = array('test', 'unset');

tests/request/request_var.php

@@ -8,12 +8,14 @@
 */
 
 require_once 'test_framework/framework.php';
-require_once '../phpBB/includes/request/request_interface.php';
+require_once '../phpBB/includes/request/type_cast_helper_interface.php';
+require_once '../phpBB/includes/request/type_cast_helper.php';
 require_once '../phpBB/includes/request/deactivated_super_global.php';
+require_once '../phpBB/includes/request/request_interface.php';
 require_once '../phpBB/includes/request/request.php';
 require_once '../phpBB/includes/functions.php';
 
-class phpbb_request_request_var_test extends phpbb_test_case
+class phpbb_request_var_test extends phpbb_test_case
 {
 	/**
 	* @dataProvider request_variables