WE'RE GETTING OOP WRONG AND THERE'S STILL TIME TO FIX IT

Hi, I'm Mark Sonnabaum

@msonnabaum

Performance engineer at Acquia

Drupal 7

• some classes
• APIs largely procedural
• not unit testable
• tightly coupled
• very complex

Drupal 8

• uses Symfony components
• many more classes
• still not unit testable
• still tightly coupled
• more complex

OOP is used to manage complexity.

How did we increase complexity?

The key to controlling complexity is a good domain model

— Martin Fowler, Domain-Driven Design

But we're not MVC?!

M is important in any design

What is a domain model?

A model is a simplification. It is an interpretation of reality that abstracts the aspects relevant to solving the problem at hand and ignores extraneous detail.

— Eric Evans, Domain-Driven Design

Domain model

• Nouns (from our ubiquitous language)
• Hold any business logic from the domain
• Ignorant of user interface, infrastructure

Drupal's Models

• node
• user
• comment
• role

• menu
• menu_link
• block
• field

• file
• vocabulary
• term
• etc…

Most of our models are

Drupal Entities

class Role extends ConfigEntityBase implements RoleInterface {
}

Do Roles truly contain no business logic?

function user_role_change_permissions($rid, array $permissions = array()) {
  // Grant new permissions for the role.
  $grant = array_filter($permissions);
  if (!empty($grant)) {
    user_role_grant_permissions($rid, array_keys($grant));
  }
  // Revoke permissions for the role.
  $revoke = array_diff_assoc($permissions, $grant);
  if (!empty($revoke)) {
    user_role_revoke_permissions($rid, array_keys($revoke));
  }
}

class Role extends ConfigEntityBase implements RoleInterface {
  public function changePermissions($rid, array $permissions = array()) {
    // Grant new permissions for the role.
    $grant = array_filter($permissions);
    if (!empty($grant)) {
      $this->grantPermissions($rid, array_keys($grant));
    }
    // Revoke permissions for the role.
    $revoke = array_diff_assoc($permissions, $grant);
    if (!empty($revoke)) {
      $this->revokePermissions($rid, array_keys($revoke));
    }
  }

How about Comments?

interface CommentInterface extends ContentEntityInterface {
}

class Comment extends EntityNG implements CommentInterface {
  public $cid;
  public $pid;
  public $nid;
  public $node_type;
  public $langcode;
  public $subject;
  // The comment author ID.
  public $uid;
  // The comment author's name.
  public $name;
  // The comment author's e-mail address.
  public $mail;
  // The comment author's home page address.
  public $homepage;
  // The comment author's hostname.
  public $hostname;
  public $created;
  // etc
}

Ambiguous property names require comments

class Comment extends EntityNG implements CommentInterface {
  // The comment author ID.
  public $uid;
  // The comment author's name.
  public $name;
  // The comment author's e-mail address.
  public $mail;
  // The comment author's home page address.
  public $homepage;
  // The comment author's hostname.
  public $hostname;

Add descriptive accessor methods

class Comment extends EntityNG implements CommentInterface {
  protected $uid;
  protected $name;
  protected $mail;
  protected $homepage;
  protected $hostname;

  public function authorId() {}
  public function authorName() {}
  public function authorEmail() {}
  public function authorHomepage() {}
  public function authorHostname() {}

If a different noun appears in multiple methods, you may need a new object.

// POPO
class CommentAuthor {
  public function id() {}
  public function name() {}
  public function email() {}
  public function homepage() {}
}

class Comment extends EntityNG implements CommentInterface {
  // Instance of CommentAuthor.
  protected $author;

  public function author() {
    return $this->author;
  }

function comment_prepare_author(Comment $comment) {
  $account = $comment->uid->entity;
  if (!$account) {
    $account = entity_create('user', array(
      'uid' => 0,
      'name' => $comment->name->value,
      'homepage' => $comment->homepage->value)
    );
  }
  return $account;
}

class Comment extends EntityNG implements CommentInterface {
  public function prepareAuthor() {
    $account = $this->uid->entity;
    if (!$account) {
      $account = new AnonymousCommentAuthor(array(
        'name' => $this->name->value,
        'homepage' => $this->homepage->value)
      );
    }
    $this->author = $account;
  }

More hidden business logic.

function comment_publish_action(Comment $comment) {

  $subject = $comment->subject->value;

  $comment->status->value = COMMENT_PUBLISHED;

  watchdog('action', 'Published comment %subject.',
    array('%subject' => $subject));
}

function comment_publish_action(Comment $comment) {
  // Knows how to find "subject"
  $subject = $comment->subject->value;
  // Knows what constant to use for it's status, sets directly
  $comment->status->value = COMMENT_PUBLISHED;

  watchdog('action', 'Published comment %subject.',
    array('%subject' => $subject));
}

Client code should not know how to publish a comment, only that it need publishing.

function comment_publish_action(CommentInterface $comment) {
  $comment->publish();

  watchdog('action', 'Published comment %subject.',
    array('%subject' => $comment->subject()));
}

// Another instance
if ($comment->status->value == COMMENT_NOT_PUBLISHED) {
  $links['comment-approve'] = array(
  ...

// Should be

if (!$comment->isPublished()) {
  $links['comment-approve'] = array(
  ...

Is publishing unique to comments?

No

Extract abstract roles from common behavior

interface PublishableInterface {
  public function publish();
  public function unPublish();
  public function isPublished();
}

interface CommentInterface extends PublishableInterface {}
interface NodeInterface extends PublishableInterface {}

But, if you move logic into Entities, you're violating the Single Responsibility Principle!!!!

In addition to the increased implementation complexity of each component, the separation immediately robs an object model of cohesion. One of the most fundamental concepts of objects is to encapsulate data with the logic that operates on that data.

— Eric Evans, Domain-Driven Design

Don't go nuts with SRP

Reasoning about responsibilities is hard

Think about reasons a class would change

Find the balance between SRP and a rich Domain Model

Many more cases of hidden business logic

Go find it, refactor it into the appropriate class

Create a new class if necessary

Types of Domain Objects

ENTITIES

(not the Drupal variety)

• has identity (uuid)
• mutable
• has state
• noun
• Drupal Entities, ConfigEntity

Attributes of an Entity can change, but it's identity remains

VALUE OBJECTS

• no identity
• immutable (treated as)
• has state
• noun
• Drupal\Core\Entity\Field\Type*

A Value object is only identifiable by it's attributes

SERVICES

• stateless
• verb-ish

Create Entities and Value Objects before Services

SERVICES should be used judiciously and not allowed to strip the ENTITIES and VALUE OBJECTS of all their behavior.

...the more common mistake is to give up too easily on fitting the behavior into an appropriate object, gradually slipping toward procedural programming.

– Eric Evans Eric, Domain-Driven Design

In general, the more behavior you find in the services, the more likely you are to be robbing yourself of the benefits of a domain model. If all your logic is in services, you've robbed yourself blind.

– Martin Fowler, AnemicDomainModel

Naming

Naming is super important

Naming is not hard

Naming classes and methods with poorly defined roles and responsibilities is hard

class AliasManager {
  // Given a path alias, return the internal path it represents.
  public function getSystemPath($path, $path_language = NULL){}
  // Given an internal Drupal path, return the alias
  public function getPathAlias($path, $path_language = NULL){}
  // Returns an array of system paths that have been looked up.
  public function getPathLookups(){}
  // Preload a set of paths for bulk alias lookups.
  public function preloadPathLookups(array $path_list){}
  public function cacheClear($source = NULL){}
  // Given a Drupal system URL return one of its aliases
  protected function lookupPathAlias($path, $langcode){}
  // Given an alias, return its Drupal system URL if one exists.
  protected function lookupPathSource($path, $langcode){}
  protected function pathAliasWhitelistRebuild($source = NULL){}
}

Terms used for non-alias paths:

• SystemPath
• internal path
• PathSource

getPathAlias

lookupPathAlias

  public function getSystemPath($path, $path_language = NULL) {
    $path_language = $path_language ?:
      $this->languageManager
        ->getLanguage(LANGUAGE_TYPE_URL)
        ->langcode;
    if ($source = $this->lookupPathSource($path, $path_language)) {
      $path = $source;
    }
    return $path;
  }

  public function getSystemPath($path, $path_language = NULL) {
    $langcode = $path_language ?: $this->defaultLanguage();
    // Contents of lookupPathAlias
  }

  protected function defaultLanguage() {
    return $this->languageManager
      ->getLanguage(LANGUAGE_TYPE_URL)
      ->langcode;
  }

Better, but vocabulary is still inconsistent

class AliasManager {
  public function findPathByAlias($alias, $path_language = NULL){}
  public function findAliasByPath($path, $path_language = NULL){}

  public function getPathLookups(){}
  public function preloadPathLookups(array $path_list){}
  public function cacheClear($source = NULL){}

  protected function pathAliasWhitelistRebuild($source = NULL){}
  protected function defaultLanguage(){}
}

getPathLookups/preloadPathLookups

are only called by AliasManagerCacheDecorator

Refactor them down

class AliasManager {
  public function findPathByAlias($alias, $path_language = NULL){}
  public function findAliasByPath($path, $path_language = NULL){}

  public function cacheClear($source = NULL){}

  protected function pathAliasWhitelistRebuild($source = NULL){}
  protected function defaultLanguage(){}
}

If there's a AliasManagerCacheDecorator, why does AliasManager need a cacheClear method?

class AliasManager {
  public function findPathByAlias($alias, $path_language = NULL){}
  public function findAliasByPath($path, $path_language = NULL){}

  protected function pathAliasWhitelistRebuild($source = NULL){}
  protected function defaultLanguage(){}
}

pathAliasWhitelistRebuild is only called by cacheClear

class AliasManager {
  public function findPathByAlias($alias, $path_language = NULL){}
  public function findAliasByPath($path, $path_language = NULL){}

  protected function defaultLanguage(){}
}

What is Drupal\Core\Path\Path?

namespace Drupal\Core\Path;

use Drupal\Core\Database\Database;
use Drupal\Core\Database\Connection;

/**
 * Defines a class for CRUD operations on path aliases.
 */
class Path {
  public function __construct(Connection $connection,
                               AliasManager $alias_manager){}
  public function save($source, $alias,
                        $langcode = LANGUAGE_NOT_SPECIFIED,
                        $pid = NULL){}
  public function load($conditions){}
  public function delete($conditions){}
}

The Path class manages CRUD for Path Aliases

Responsibility overlap

Refactor into AliasManager

namespace Drupal\Core\Path;

class AliasManager {
  public function findPathByAlias($alias, $path_language = NULL){}
  public function findAliasByPath($path, $path_language = NULL){}
  public function findWhere($conditions){}
  public function deleteWhere($conditions){}
  public function save($path, $alias,
                        $langcode = LANGUAGE_NOT_SPECIFIED,
                        $pid = NULL){}

  protected function defaultLanguage();
}

We have a culture of emphasizing systems over the Domain

Our User model:

\Drupal\user\Plugin\Core\Entity\User

Understanding the User

model requires knowledge of

both Entity and Plugin

What it could be

\Drupal\user\EntityType\User

What I'd prefer

\Drupal\user\User

namespace Drupal\custom_block\Controller;

class CustomBlockController implements ControllerInterface {
  public function add() {
    // some code
    $types = $this->entityManager
                  ->getStorageController('custom_block_type')
                  ->load();

CustomBlockController is working within the domain of CustomBlocks

Why does it need to know anything about an EntityManager?

namespace Drupal\custom_block\Controller;

class CustomBlockController implements ControllerInterface {
  public function add() {
    // some code
    $types = CustomBlock::findAll();

Or if you're fancy

namespace Drupal\custom_block\Controller;

class CustomBlockController implements ControllerInterface {
  public function add() {
    // some code
    $types = $this->customBlockRepository->findAll();

Controllers

class AggregatorController implements ControllerInterface {

  public function adminOverview() {
    $result = $this->database->query('SELECT f.fid, ...');

    $header = array(t('Title'), t('Items'); // etc…
    $rows = array();
    foreach ($result as $feed) {
      $row = array();

    $result = $this->database->query('SELECT f.fid, ...');

Find business logic in Controllers, move it into Domain Objects

class AggregatorController implements ControllerInterface {

  public function adminOverview() {
    $feeds = new AggregatorFeeds();

    foreach ($feeds->findAll() as $feed) {
      $row = array();

Create simple accessors for Domain Objects to simplify Controller logic further

class AggregatorController implements ControllerInterface {

  public function adminOverview() {
    foreach ($this->feeds()->findAll() as $feed) {
      $row = array();
      // more stuff
  }

  protected function feeds() { 
    return $this->feeds ?: new AggregatorFeeds();
  }

Dumb down Controllers until they aren't worth unit testing

Unit Testability

&

Coupling

namespace Drupal\Tests\Core\TypedData\Type;

use Drupal\Core\TypedData\Type\Map;
use Drupal\Tests\UnitTestCase;

class MapTest extends UnitTestCase {
  public function testIteration() {
    $value = array('one' => 'eins', 'two' => 'zwei');
    $this->map = new Map(array('type' => 'map'), $value);

    $count = 0;
    foreach ($this->map as $item) {
      $this->assertTrue($item instanceof
        \Drupal\Core\TypedData\TypedDataInterface);
      $count++;
    }
    $this->assertEquals(2, $count);
  }

1) Drupal\Tests\Core\TypedData\Type\MapTest::testIteration
Failed asserting that 0 matches expected 2.

namespace Drupal\Tests\Core\TypedData\Type;

use Drupal\Core\TypedData\Type\Map;
use Drupal\Tests\UnitTestCase;

class MapTest extends UnitTestCase {
  public function testIteration() {
    $value = array('one' => 'eins', 'two' => 'zwei');
    $this->map = new Map(array('type' => 'map'), $value);
    $this->map->setValue($value, FALSE);
    $count = 0;
    foreach ($this->map as $item) {
      $this->assertTrue($item instanceof
        \Drupal\Core\TypedData\TypedDataInterface);
      $count++;
    }
    $this->assertEquals(2, $count);
  }

2. Fatal error: Call to undefined function cache() in
core/lib/Drupal/Core/Plugin/Discovery/CacheDecorator.php
on line 135

3. Fatal error: Call to undefined function module_implements() in
core/lib/Drupal/Core/Plugin/Discovery/HookDiscovery.php
on line 48

4. Fatal error: Call to undefined function format_string() in
core/lib/Drupal/Core/TypedData/TypedDataFactory.php
on line 44

throw new InvalidArgumentException(
  sprintf('Invalid data type %s has been given.', $plugin_id)
);

5. PHP Fatal error:  Call to undefined function typed_data() in
core/lib/Drupal/Core/TypedData/Type/Map.php
on line 119

- $this->properties[$property_name] = typed_data()->
-   getPropertyInstance($this, $property_name, $value);
+ $definition = $this->getPropertyDefinition($property_name);
+ $manager = new \Drupal\Core\TypedData\TypedDataManager;
+ $this->properties[$property_name] = $manager->create(
+   $definition, NULL, $property_name, $this
+ );

Finally, it passes.

Inheritence

hardcoding a dependency

Base classes have the responsibility to not

screw their subclasses

Map screwed by:

class Entity {
  public function save() {
    return \Drupal::entityManager()
      ->getStorageController($this->entityType)->save($this);
  }
}

Bare minimum

hide each dependency

in a method

class Entity {
  public function save() {
    return $this->getStorage->save($this);
  }

  protected function getStorage() {
    if (!isset($this->storage)) {
      $this->storage = \Drupal::entityManager()
        ->getStorageController($this->entityType);
    }
    return $this->storage;
  }
}

SRP is equally important to apply to methods

Our code is still

tightly coupled

If you can't use PHPUnit to test your class

You cannot claim that it is loosely coupled

DrupalUnitTestBase is not a unit test

UnitTestBase is also not a unit test

class RouteProviderTest extends UnitTestBase {
 /**
   * Confirms that the correct candidate outlines are generated.
   */
  public function testCandidateOutlines() {
    $connection = Database::getConnection(); // <-- WTF
    $provider = new RouteProvider($connection);

Go convert tests to PHPUnit and prove me wrong

There is still time to improve our Domain model

Most changes are just refactorings

Let's make Drupal simpler

Sprint: Friday

Follow @drupalmentoring

http://portland2013.drupal.org/program/sprints

Recommended reading

Thanks.