Eating ElePHPants



Eating ElePHPants

0 0


slides-turning-big-ships


On Github Crell / slides-turning-big-ships

Eating ElePHPants

Presented by Larry Garfield (@Crell)

@Crell

  • Senior Architect, Palantir.net
  • Drupal 8 Web Services Lead
  • Drupal Representative, PHP-FIG
  • Advisor, Drupal Association
  • implements Huggable
Who's excited to write code like this?
Going home to this?

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".
  • 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
  • 8139 unit tests (and counting)
  • Flexible architecture
  • Modern standards
  • Ebraces PIE
  • >3x contributors (3000+ 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

  • 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
Plans are worthless, but planning is everything.

—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

Pros

  • Low-hanging fruit
  • Easier parallelism
  • Unravel knots bit by bit

Cons

  • Low-incentive
  • Less impactful
  • Staving off the big break

Top down

Pros

  • Highest-visibility first
  • Identify the major knots
  • Better contrast w/ old code

Cons

  • Higher risk
  • So many knots…
  • Don't forget the low-fruit!
  • High risk = Not enough experience yet.
  • low-fruit: drupal_set_message() is still a function. WTF?
The right approach technicallyand the right approach politicallymay 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

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

  • "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
"if you give 3 people 3 days to work together on something, they’ll get unbelievable amounts of work done."

— Tim Plunkett, Drupal 8 developer #2

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

  • 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

Writing code should not be the 1st response. Finding if shared code exists should be the 1st response.

Beth Tucker-Long, Code Climate

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)

s/drupal_http_request()/Guzzle/

  • 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

When you commit enough regressions, it gets harder to measure new ones.

—Nathaniel Catchpole (catch), Drupal 8 Release Manager

  • OOP code isn't slower
  • Refactoring can break performance
The Best is the Enemy of Good

—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

FormBuilder is gross, but injectable

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

"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

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

This is an ongoing process

One bite at a time, with friends

If we can do it, so can you

Larry Garfield

Senior Architect, Palantir.net

Making the Web a Better Place

Keep tabs on our work at @Palantir

Want to hear about what we're doing?

The story of Drupal: * D7 * D8 * elephpants * WSCCI * Learn from our mistakes Plan ahead (mgmt): Mesaging matters (social) Beachhead (social) Structure: * Undivided attention (social) * Bikeshed (social) Preparing: * Don't write unit tests (tech) * Outsourcing is good for you (technical) * Tools (technical) Incremental progress (tech) * Use an SL (technical) * Break into steps (tech) * Perfectionism (technical) * Sisyfus cat * It's hard, but if we can do it so can you * One bite at a time, with friends
Eating ElePHPants Presented by Larry Garfield (@Crell)