Web-stories-wp: PHP Refactoring to facilitate testing and 3P plugin integrations

[swissspidy]

Feature Description

When working on #113 and #1117 it has become apparent that the PHP refactoring originally tentatively planned for post-stable is becoming more urgent.

We currently use a colorful mix of static class methods, and classes instantiated at various stages without the possibility to easily modify their behavior.

Example:

For #113, we'd need to be able to remove all the filters added by the Discovery class. However, this is currently not easily possible.

This is a good opportunity to reduce technical debt on the PHP side of things.

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance Criteria

• More robust but not overly complex plugin architecture
  Ideally first drafted in a design doc or proof-of-concept PR
• 3P plugin integrations are more easily possible

Implementation Brief

[spacedmonkey]

The current PHP code, relays to much on static methods and class consts. It makes for extremely messy code and hard to understand dependency tree. There is also poor use of Object-oriented programming principles.

I have already had some thoughts on the refactor. I think we have 2 options.

1. Refactor the code to namespaces + functions.
   Simple don't use objects at all and use namespaces functions.

This gives some benefits.

• Simple to understand and maintain code.
• Functions are very re-usable
• Simple to understand for javascript developers.
  Pitfalls.
• Old fashioned code style.
• Requires use of globals to track state.
• Not how PHP is normally used.

1. Refactor the code to use objects, dependency injection ( with name spaces ).

I have had some great success with this pattern. Take the following example.

class Plugin{
     public $example;
     public $settings;

     public function init() {
          $this->example = new Example($this);
          $this->example->init();

          $this->settings = new Settings( $this );
      $this->settings->init();
    }
}

class Example{
      protected $plugin;

     public function __construct( $plugin ) {
            $this->plugin = $plugin;
    }

    public function init(){
        add_action('init', [$this, 'test]);
    }

    public function test(){
       echo $this->settings->get_option();
    }
}

This model is very clean, as each object has it's purpose. But we are using dependency injection, in test, it is easy to make the plugin object passed to example pass.

This should stop requiring the need for static methods and consts. As class varibles should be passed via the plugin object.

I also think that trait and parent classes could be used to reuse code.

These are just some thoughts, it might be worth making a design doc.

[spacedmonkey]

Might be worth using a tool like this to see how the code is laid out - https://github.com/mamuz/PhpDependencyAnalysis

[swissspidy]

@spacedmonkey Thanks for writing this up!

When we first started with this plugin I was leaning more towards something like the former option, to keep everything simpler. However, the code base is inevitably growing, and more PHP functionality is being added more quickly than I originally anticipated.

The current code is indeed not great, but in the spirit of fast iterations it was an acceptable tradeoff. Now, I think proper OOP with DI makes sense.

Have you seen the https://github.com/google/site-kit-wp/ source code? It's proven to be working well, and I feel like we could quickly adapt that for us. No need to reinvent the wheel. WDYT?

[spacedmonkey]

Looking at the sitekit source code, is does similarly weird things that this code base does like this.

This has nested functions. It is hard to read and even harder to write unit tests. I am not sure this is the best model to follow. I also think that having anonymous functions hooked into filters and actions is bad practice for plugins. It makes it impossible to remove that filter.

What was documented above is how the XWP template plugin ( called wp-foo-bar ) works. Many of XWP plugins are been build using the template with great success.

Setup of the plugin class looks like this.
Here is a sample sub class looks like this.

I honestly don't think it would take that long to refactor, we already have a central plugin file. I think we just need to the following.

• Add class variables for all the classes.
• Pass plugin instance to constructor of all other classes.
• Move code from anonymous to class methods.
• Make some methods public to make the testable or use reflectionmethod in tests.
• Add getters for Class constants ( to support older PHP versions ).
• Convert all places in code where classes are cross referenced.

This sounds like a lot, but I think I could turn this around very quickly.

[swissspidy]

Looking at the sitekit source code, is does similarly weird things that this code base does like this.
It makes it impossible to remove that filter.

Yeah I think this exact pattern we'd want to avoid 🙂

We _do_ want to make it possible to remove filters. That's the key point for this and #113.

I think we just need to the following. [...]

Seems like a good direction.

@spacedmonkey As a proof-of-concept could you perhaps create a PR that rewrites the Discovery class as per your vision?

I want to validate that:

1. this approach works
2. this allows other plugins like Yoast to remove filters

Also, WDYT of using a Context instance like Site Kit does and passing that around (instead of Plugin)?

[spacedmonkey]

@spacedmonkey As a proof-of-concept could you perhaps create a PR that rewrites the Discovery class as per your vision?

Discovery is a bad example. It it already pretty OOP. The only thing I would change is remove some of static methods. See this. I think media is a good example, as this has lots of static methods.

Also, WDYT of using a Context instance like Site Kit does and passing that around (instead of Plugin)?

It seems to be the same thing, but with a different name :P

[spacedmonkey]

As talking point, I have done a little POC, https://github.com/google/web-stories-wp/pull/2588
I have added comments to understand what I have done here.

[spacedmonkey]

I did a little research into good pattern of plugins design in the WordPress space.I still think that wp-foo-bar is the better pattern. It avoid some bad practice in PHP development.

• Single responsibility classes.
• No singletons
• Avoids static methods.
• Avoid anonymous functions.
• Uses namespaces.

[swissspidy]

@spacedmonkey Is there anything left here that you think we should tackle?

[spacedmonkey]

I think we could improve testing, but I think that should be it's own ticket. I think we should reuse https://github.com/google/web-stories-wp/issues/328 for this and expane it's scope.

I am going to move this to done now.