Evolution is possible!

Target Testable Code

• "In what direction do we evolve?" Toward testability

"Testable" is a proxy for ...

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

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

• Testability is a stand-in for "good".

Very powerful, but straining at the seams

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

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

• Wanted to do the new stuff "the right way" for its own sake
• Let it ripple out as it would
• We can't be the first to do this...

• HttpFoundation first, then Kernel, then snowball

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/governancechallenge than a technical challenge

• True of OSS and company development
• Leadership buy-in needed
• Subterfuge possible, short-lived

—Dwight Eisenhower

Are you refactoringor adding refactored features?

• Drupal wanted features; new-style was means to end
• Some people taken by surprise by refactoring
• Refactoring for its own sake came later

People need to see value

Developers need to see value

Management need to see value

• Drupal has management; Dries, Angie, companies, etc.
• You need buy-in

Is refactoring valuable?

Code quality doesn't matter, only user experience matters

Wrong

Developers are users... of code

• UX depends on developer's ability to implement; bad code, no implementation
• Technical debt limits ability to develop

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

• Very successful; way easier for devs than manual

The big bite

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

Throw HttpKernel in the middleand let the chips fall where they may

• Old router only knew paths
• Symfony/CMF router knows full request
• The world didn't need another Request object

Make the old code feel clunkierin comparison

Shame the old code, not the old coders

• Half of it was your code anyway
• You want friends helping, not fighting you

Bottom up

Top down

• High risk = Not enough experience yet.
• low-fruit: drupal_set_message() is still a function. WTF?

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

• Blogs, IRC, pub. discussions, conferences
• Took a year for people to stop asking "so what is WSCCI?"
• Politicians sound like canned responses because they are canned

There are costs to poor messaging...

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

• Extreme cases, fork; Backdrop
• Will you get more people than you lose? I think yes.

—General Robert H. Barrow, US Marine Corps

—Larry Garfield, 2011 and 2012 and 2013 and 2014

Work does not happen in spare time

• "In-house projects" happen when?
• Nights and weekends SUCK.
• Meetings 1 hour increments; work 4 hour increments

Sprints

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

• Drupal one of the largest distrib teams in the world
• We work best in person

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?

• Eventually JSON, because easy
• Moved to YAML, because docs/UTF-8
• XML would have been better long-run, but too much hate

Bikeshed stoppers

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

Luck

• Strong leaders know how/when to say Shut Up in a way people appreciate
• Explain reasons behind everything
• So now that you're doing it, how do you do it, tactically?

—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

• Pre-Travis et al
• Pre-Behat

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

• Composer, Symfony, Zend, Aura: Components FTW!
• Systems not built from components suck; replace them

That's the point

For new functionality

Drupal never seriously considered writing a YAML parser

• New Configuration system, uses YAML
• Uses Symfony YAML

Share with yourself

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

• \Drupal\Component
• Keeping it in sep package forces decoupling
• Drupal Components hard to keep clean

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)

• Guzzle 3 refactored FOR US

However!

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

• Outsourcing has costs; lack of control
• It often has value... but not always
• Still using own DBAL

You want 100% green tests?

CI server or it won't happen

• Testbot sucks, but it saves our butt daily.
• Travis?

Performance

• OOP code isn't slower
• Refactoring can break performance

—My dad (and Voltaire)

Idealism is a guide, not a rule

Incremental progress is progress

• Hard to remember when you don't know when your deadline is.

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?

• An SL is better than functions
• One global is better than many

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');
  }

  // ...
}

• Only one global
• For procedural code only!

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();
  }
}

• Still ugly, but the code it calls doesn't have to be.

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];
  }
}

• No injection, legacy procedural code

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();
  }
}

• Now just need to inject and change 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;
  }
}

• And we're done.

Service locators are a stepping stonetoward Dependency Injection

Break things in steps

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

• BC shim = Backward compatibility layer
• Old func calls the new func

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 usemust be acceptable

Improving code without adding features must be acceptable

Time for just paying down debt must

• Rearranging deck chairs on the Titanic is OK if they get you closer to a life boat
• Early: "No new features, so why do it?" -- Slowed us down a lot
• Code improvement has to be seen as intrinsically valuable
• Management problem if not...

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

• 10 dependencies(!)

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

• Patches ready and waiting to simplify.

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

— @Getify

Interfaces FTW

Refactor the fugly to be smaller

• Getting to clean interfaces is worth fighting for
• Cleaning up implementation is next; sometimes fight