Eating ElePHPants

Evolution is possible!

Target Testable Code

"Testable" is a proxy for ...

• Loosely coupled
• Decoupled
• Injectable
• Reusable
• Understandable
• Clean
• "Good"

If it's hard to test, you're doing it wrong

• PHP4-era Procedural
• Array-oriented
• Lots of functional tests
• "Idiomatic" (but extensible) architecture
• Runs 2% of the web
• Massive community (954 contributors)

Very powerful, but straining at the seams

• Architecturally OOP
• 8506 unit tests (and counting)
• Flexible architecture
• Modern standards
• Embraces PIE
• >3x contributors (3136 and counting)

How'd they do that?

How does one eat an elePHPant?

One bite at a time...

... with friends

… with lots of friends

WSCCI

Web Services and Context Core Initiative

Drupal needs to evolve, and quickly, from a first-class web CMS into a first-class REST server that includes a first-class web CMS.

Refactoring everything wasn't a goal, it was a methodology

Proudly Invented Elsewhere

• Symfony2 (HttpFoundation, HttpKernel, DependencyInjection, EventDispatcher, Routing, Serializer, Validator, Yaml)
• Symfony CMF
• Zend Feed
• Doctrine Annotations
• Guzzle
• EasyRDF
• Twig
• PHPUnit
• Zend Diactoros

Learn from our experience mistakes

Refactoring is more a social/governance
challenge than a technical challenge

Plans are worthless, but planning is everything.

—Dwight Eisenhower

Are you refactoring
or adding refactored features?

People need to see value

Developers need to see value

Management need to see value

Is refactoring valuable?

Code quality doesn't matter, only user experience matters

Wrong

Developers are users... of code

The new code will be better

I'll believe it when I see it

The first taste

• Drupal 7 DBAL
• Home grown (Sorry, Doctrine)
• 98% OOP
• See, it's not so scary

c. 2008-2011

The big bite

• Routing / Web services
• REST-ify routing
• Lots of Symfony code

Throw HttpKernel in the middle
and let the chips fall where they may

Make the old code feel clunkier
in comparison

Shame the old code, not the old coders

Bottom up

Top down

The right approach technically
and the right approach politically
may be completely different.

Clear messaging

Repeat your message

Everyone wants to hear the message themselves

Need to repeat it in different ways

It's a total waste of time... Do it anyway

There are costs to poor messaging...

• People learn at different rates
• You're moving my cheese!
• Not everyone will make the transition...

Amateurs think about tactics,
but professionals think about logistics.

—General Robert H. Barrow, US Marine Corps

Give me 5 people 10 hours a week and I can do anything. Give me 50 people one hour a week and I can't do squat.

—Larry Garfield, 2011 and 2012 and 2013 and 2014

Work does not happen in spare time

Sprints

• In person collaboration
• Small group
• Right people
• Dedicated time

Beware the bikeshed

Configuration system

• Feature change #1
• Staging & Deployment: Solved!
• June 2011: 3 day, 5 person sprint, Denver
• Detailed recommendation
• Files on disk, load system, API, staging, etc.
• Format: JSON, YAML, XML?

Bikeshed stoppers

• Strong leadership
• Clear decision making process
• Over-communication

Luck

Proper prior planning prevents piss poor performance

—British military adage

Don't write unit tests

Say WAT?

Write system tests

• If your code were already unit testable we'd be done
• Unit tests change with the unit
• Unit test the new stuff
• Don't break user functionality
• 100% green at all times

Drupal wrote its own testing framework

(Hey, it was 2008)

Having those tests saved our butts

Functional tests are for functionality... only

Pseudo-unit tests are worse than no tests

Sharing is caring

I don't want to maintain [my] crappy code!

So use someone else's

(Then they have to maintain it!)

That's the point

For new functionality

Drupal never seriously considered writing a YAML parser

Share with yourself

• Build new code as components
• Write it right
• Share on Packagist!

For existing functionality

Refactor your app, don't replace it

Replace your component, don't refactor it

drupal_http_request()

• HTTP/1.0 client
• One 304 line function (No, really)
• N-Path complexity: 25,303,344,960
• Lacking a number of features (proxy support)

s/drupal_http_request()/Guzzle/

However!

If it's not broke, don't fix it

You want 100% green tests?

CI server or it won't happen

Performance

The Best is the Enemy of Good

—My dad (and Voltaire)

Idealism is a guide, not a rule

Incremental progress is progress

User a Service Locator

Larry, you so funny...

But Service Locators are eeeeevil!

Service Locator = passed DI Container

That's it

Remember what we said about small steps?

class Drupal {
  protected static $container;

  public static function setContainer(ContainerInterface $container = NULL) {
    static::$container = $container;
  }

  public static function getContainer() {
    return static::$container;
  }

  public static function service($id) {
    return static::$container->get($id);
  }

  public static function entityManager() {
    return static::$container->get('entity.manager');
  }

  // ...
}
        

function menu_menu_update(Menu $menu) {
  menu_cache_clear_all();
  // Invalidate the block cache to update menu-based derivatives.
  if (\Drupal::moduleHandler()->moduleExists('block')) {
    \Drupal::service('plugin.manager.block')->clearCachedDefinitions();
  }
}
        

class BookManager {

  public function bookTreeAllData($bid, $link = NULL, $max_depth = NULL) {
    $language_interface = \Drupal::languageManager()->getCurrentLanguage();
    // ...
    $tree[$cid] = $this->bookTreeBuild($bid, $tree_parameters);

    return $tree[$cid];
  }
}
        

class BookManager {

  public function bookTreeAllData($bid, $link = NULL, $max_depth = NULL) {
    $language_interface = $this->languageManager()->getCurrentLanguage();
    // ...
    $tree[$cid] = $this->bookTreeBuild($bid, $tree_parameters);

    return $tree[$cid];
  }

  public function languageManager() {
    return \Drupal::languageManager();
  }
}
        

class BookManager {

  public function __construct(LanguageManager $language_manager) {
    $this->languageManager = $language_manager
  }

  public function bookTreeAllData($bid, $link = NULL, $max_depth = NULL) {
    $language_interface = $this->languageManager()->getCurrentLanguage();
    // ...
    $tree[$cid] = $this->bookTreeBuild($bid, $tree_parameters);

    return $tree[$cid];
  }

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

Service locators are a stepping stone
toward Dependency Injection

Break things in steps

• BC shims are your friend
• Deprecate immediately
• Convert BC shims

Nothing is more permanent than a temporary solution

This takes commitment to address

Temporary solutions

• @deprecated (Never add uses)
• Change notices (and Documentation!)
• Peer review
• E_USER_DEPRECATED?
• Sunset plan: release blocking?

Changesets that just remove legacy use
must be acceptable

Improving code without adding features must be acceptable

Time for just paying down debt must be acceptable

Don't bite off more than you can chew

Session handling

• 2 years on 1 patch... fail :-(
• Incremental steps: 10+ patches…
• … across 18 months

Finally!

Procedural code inside class keyword is still progress

Form Builder

class FormBuilder {
  public function __construct(FormValidatorInterface $form_validator, FormSubmitterInterface $form_submitter, FormCacheInterface $form_cache, ModuleHandlerInterface $module_handler, EventDispatcherInterface $event_dispatcher, RequestStack $request_stack, ClassResolverInterface $class_resolver, ElementInfoManagerInterface $element_info, ThemeManagerInterface $theme_manager, CsrfTokenGenerator $csrf_token = NULL) {
    $this->formValidator = $form_validator;
    $this->formSubmitter = $form_submitter;
    $this->formCache = $form_cache;
    $this->moduleHandler = $module_handler;
    $this->eventDispatcher = $event_dispatcher;
    $this->requestStack = $request_stack;
    $this->classResolver = $class_resolver;
    $this->elementInfo = $element_info;
    $this->csrfToken = $csrf_token;
    $this->themeManager = $theme_manager;
  }

  // ...

  public function getFormId($form_arg, &$form_state) {
    // 20 lines of fugly here.
  }

  public function buildForm($form_id, array &$form_state) {
    // 100 lines of code/comments here
    return $form;
  }
}
        

form.inc

/**
 * @deprecated
 */
function drupal_get_form($form_arg) {
  return call_user_func_array(
    [\Drupal::formBuilder(), 'getForm'],
    func_get_args()
  );
}

/**
 * @deprecated
 */
function drupal_build_form($form_id, &$form_state) {
  return \Drupal::formBuilder()->buildForm($form_id, $form_state);
}

/**
 * @deprecated
 */
function form_state_defaults() {
  return \Drupal::formBuilder()->getFormStateDefaults();
}
        

Later removed entirely!

No program will ever be perfect, just make it better

Contain the crap

Form Builder

class FormBuilder {
  public function __construct(FormValidatorInterface $form_validator, FormSubmitterInterface $form_submitter, FormCacheInterface $form_cache, ModuleHandlerInterface $module_handler, EventDispatcherInterface $event_dispatcher, RequestStack $request_stack, ClassResolverInterface $class_resolver, ElementInfoManagerInterface $element_info, ThemeManagerInterface $theme_manager, CsrfTokenGenerator $csrf_token = NULL) {
    $this->formValidator = $form_validator;
    $this->formSubmitter = $form_submitter;
    $this->formCache = $form_cache;
    $this->moduleHandler = $module_handler;
    $this->eventDispatcher = $event_dispatcher;
    $this->requestStack = $request_stack;
    $this->classResolver = $class_resolver;
    $this->elementInfo = $element_info;
    $this->csrfToken = $csrf_token;
    $this->themeManager = $theme_manager;
  }
}
        

The code around it is fine

Know when to push for better, and when it's good enough

"perfect is the enemy of good". yeah, but good enough is the enemy of good, too.

— @Getify

Interfaces FTW

Refactor the fugly to be smaller