Refactoring session class to make it more testable

General discussion of development ideas and the approaches taken in the 3.x branch of phpBB. The current feature release of phpBB 3 is 3.3/Proteus.
Forum rules
Please do not post support questions regarding installing, updating, or upgrading phpBB 3.3.x. If you need support for phpBB 3.3.x please visit the 3.3.x Support Forum on phpbb.com.

If you have questions regarding writing extensions please post in Extension Writers Discussion to receive proper guidance from our staff and community.
Post Reply
asperous
Google Summer of Code Student
Posts: 21
Joined: Mon Apr 22, 2013 3:26 pm
Location: Tigard, Or
Contact:

Refactoring session class to make it more testable

Post by asperous »

So in order to make the session class more testable it would be advantageous to decouple some of the global state variablables out of functions that perform logic.

Before:

Code: Select all

static function extract_current_page($root_path)
{
  global $request;

  $page_array = array();

  // First of all, get the request uri...
  $script_name = htmlspecialchars_decode($request->server('PHP_SELF'));
  $args = explode('&', htmlspecialchars_decode($request->server('QUERY_STRING')));

  if (!$script_name)
  {
    $script_name = htmlspecialchars_decode($request->server('REQUEST_URI'));
    $script_name = (($pos = strpos($script_name, '?')) !== false) ? substr($script_name, 0, $pos) : $script_name;
    $page_array['failover'] = 1;
  }
  ...
After:

Code: Select all

static function extract_current_page($root_path)
{
      global $request;
      $server = $request->server;
      return self::extract_current_page_($root_path, $server('PHP_SELF'), $server('QUERY_STRING'), $server('REQUEST_URI'));
  }

static function extract_current_page_with_variables($root_path, $script_name, $query_string, $request_uri)
{
  $page_array = array();

  // First of all, get the request uri...
  $script_name = htmlspecialchars_decode($script_name);
  $args = explode('&', htmlspecialchars_decode($query_string));

  if (!$script_name)
  {
    $script_name = htmlspecialchars_decode($request_uri);
    $script_name = (($pos = strpos($script_name, '?')) !== false) ? substr($script_name, 0, $pos) : $script_name;
    $page_array['failover'] = 1;
  }
  ...

Pros:
- Decoupled means $request->server is easier to change in the future
- Less global state in the function means input/output is more predictable
- By simply splitting out the state but keeping the same interface ensures compatibility
- Easier to write tests, function inputs map to outputs (with no side-effects)
- More comprehensive tests, since some global objects are read-only

Cons:
- More lines of code (bloats class)
- Pollutes name space with functions that end with _with_variables (or _underlying or __)
- Tests that I write will require this these changes to work

Alternatively I could just try to write tests with the existing functions, but they will not be as comprehensive.
User avatar
bantu
3.0 Release Manager
3.0 Release Manager
Posts: 557
Joined: Thu Sep 07, 2006 11:22 am
Location: Karlsruhe, Germany
Contact:

Re: Refactoring session class to make it more testable

Post by bantu »

Think of global variables as another (pretty bad) way of passing data into a method/function. You can write to global variables in unit tests just like you can pass parameters to the function/method/class you are testing, so testability does not seem to be a big issue. There may be a few corner cases where testing is hard or even impossible, but it's not hard in general. These corner cases can be changed/fixed on a case-by-case basis. Your proposal is basically some refactoring and refactoring is probably something you'll have to do anyway in order to end up with a proper session interface. But before you can do some refactoring, we really want code coverage, so it seems to be best to cover the existing code as it is.
Post Reply