Eating ElePHPants

What we want

No legacy code

Less legacy code

Legacy code

• noun: Code I didn't write
• noun: Code without unit tests
• noun: Code worth testing that cannot be unit tested

What we really want is testable code

Evolution is possible!

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

Very powerful, but straining at the seams

• Architecturally OOP
• 4600 unit tests (and counting)
• Flexible architecture
• >2x contributors (2030 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
• Assetic
• Twig
• PHPUnit

Learn from our experience mistakes

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

Fire anyone who says this

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

Work does not get done in spare time

Context switching sucks

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

Sprints

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

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, solid communication, luck

Proper prior planning prevents piss poor performance

—British military adage

Don't write unit tests

Say WAT?

Write integration tests

• If your code were already 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

(Please please do not do this)

Having those tests saved our butts

Functional tests are for functionality... only

Pseudo-unit tests are worse than no tests

Sharing is caring

Modern PHP is all about sharing

It's way easier than it used to be

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

For existing functionality

Refactor your app, don't replace it

Replace your component, don't refactor it

(Especially cryptography!)

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)

Use Guzzle instead

We did our research!

• HTTP/1.1
• Well-tested
• Loosely coupled
• Widely used
• Way better than ours

s/drupal_http_request()/Guzzle/

However!

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

Share with yourself

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

You want 100% green tests?

CI server or it won't happen

Performance

Make code reviews easy

• Dreditor
• GitHub Pull Requests
• Please don't use patches...

Make contributing easy

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 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

Big refactoring breaks APIs

Deal

How to deal

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

Drupal 7

function cache_get($cid, $bin = 'cache') {
  return _cache_get_object($bin)->get($cid);
}

function cache_get_multiple(array &$cids, $bin = 'cache') {
  return _cache_get_object($bin)->getMultiple($cids);
}

function cache_set($cid, $data, $bin = 'cache', $expire = CACHE_PERMANENT) {
  return _cache_get_object($bin)->set($cid, $data, $expire);
}
            

Drupal 8 (early)

/**
 * @deprecated
 */
function cache_get($cid, $bin) {
  return cache($bin)->get($cid);
}

function cache($bin = 'cache') {
  return \Drupal::cache($bin);
}
            

function something() {
  $item = cache_get('menu', 'a_key');
}
            

function something() {
  $item = cache()->get('a_key');
}
            

function something() {
  $item = \Drupal::cache('menu')->get('a_key');
}
            

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

Don't bite off more than you can chew

Session handling

• 2 years on 1 patch... fail :-(
• Rearrange deck chairs: 2 in already.
• We'll get there...

Procedural code inside class keyword is still progress

Form Builder

class FormBuilder {
  public function __construct(ModuleHandlerInterface $module_handler, KeyValueExpirableFactoryInterface $key_value_expirable_factory, EventDispatcherInterface $event_dispatcher, UrlGeneratorInterface $url_generator, TranslationInterface $translation_manager, CsrfTokenGenerator $csrf_token = NULL, HttpKernel $http_kernel = NULL) {
    $this->moduleHandler = $module_handler;
    $this->keyValueExpirableFactory = $key_value_expirable_factory;
    $this->eventDispatcher = $event_dispatcher;
    $this->urlGenerator = $url_generator;
    $this->translationManager = $translation_manager;
    $this->csrfToken = $csrf_token;
    $this->httpKernel = $http_kernel;
  }

  // ...

  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

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

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

function form_state_defaults() {
  return \Drupal::formBuilder()->getFormStateDefaults();
}

function drupal_rebuild_form($form_id, &$form_state, $old = NULL) {
  return \Drupal::formBuilder()->rebuildForm($form_id, $form_state, $old);
}
            

No program will ever be perfect, just make it better

Contain the crap

Form Builder

class FormBuilder {
  public function __construct(ModuleHandlerInterface $module_handler, KeyValueExpirableFactoryInterface $key_value_expirable_factory, EventDispatcherInterface $event_dispatcher, UrlGeneratorInterface $url_generator, TranslationInterface $translation_manager, CsrfTokenGenerator $csrf_token = NULL, HttpKernel $http_kernel = NULL) {
    $this->moduleHandler = $module_handler;
    $this->keyValueExpirableFactory = $key_value_expirable_factory;
    $this->eventDispatcher = $event_dispatcher;
    $this->urlGenerator = $url_generator;
    $this->translationManager = $translation_manager;
    $this->csrfToken = $csrf_token;
    $this->httpKernel = $http_kernel;
  }
}
            

The code around it is fine

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

Interfaces FTW

Refactor the fugly to be smaller