Make WordPress Core

Opened 9 years ago

Closed 6 years ago

Last modified 5 years ago

#32470 closed enhancement (invalid)

Abstracting the Widget Classes

Reported by: welcher's profile welcher Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Widgets Keywords: dev-feedback 2nd-opinion bulk-reopened
Focuses: Cc:

Description

Starting a discussion on the best way to abstract out the functionality of WP_Widget and the built in widgets in default-widget.php.

Related #23012

Attachments (5)

32470.diff (11.2 KB) - added by welcher 9 years ago.
32470.2.diff (11.0 KB) - added by welcher 9 years ago.
Corrected diff
32470.3.diff (5.2 KB) - added by welcher 9 years ago.
Functional changes to WP_Widget and using new methods in default-widget.php
32470.4.diff (5.8 KB) - added by welcher 9 years ago.
Adding verify_settings() and widget_title() methods
32470-interface.patch (7.3 KB) - added by jacobsantos 9 years ago.
32470.4.diff using interfaces

Download all attachments as: .zip

Change History (39)

#1 follow-up: @jacobsantos
9 years ago

I recommend not doing it any existing way in WordPress. I die a little every time I read WordPress code. It is like, "Do you even OOP, brah?" and the answer is no.

Basically, SOLID, check Wikipedia. There are books as well.

  • Use interfaces (allows dependency injection).
  • Smaller independent classes (Best Practice).
  • Smaller interfaces (Best Practice).
  • Type hint interfaces or check instanceof (Dependency Injection).
  • Attempt to move away from inheritance and more towards composition.
  • Don't ever, never ever. Never ever? Yes, never ever use static methods, unless you are creating a singleton (best practice).
  • If the question is, "Should I use a singleton?" The answer is no. If the question is, "Are you sure I shouldn't use a singleton?" The answer is still no. If the question is still, "I think I should use a singleton." then okay, you obviously know what you are doing, so go ahead (best practice).
  • Classes should be small (best practice and Single Responsibility).
  • Methods should be small (best practice).
  • Views should be in a separate file (separation of concerns and best practice).
  • Classes should only contain unique code (Single Responsibility and obviously, best practice).
  • If the answer is "I could just extend this class to change the functionality." Then you are asking the wrong question. You are better off creating a new interface and moving the code you were going to add by extending to a new class and dependency injecting the class that matches the new interface.

The end result will be greater amount of smaller classes with interfaces that describe functionality that can be called in functions and other objects. No object should reference a class by name, only check that the class implements interfaces.

Last edited 9 years ago by jacobsantos (previous) (diff)

#2 follow-up: @westonruter
9 years ago

Perhaps the biggest thing that bugs me about WP_Widget is that one instance of this object does not correspond to one instance in the database, as in you always have to pass in the $args and $instance with each call because it doesn't know which database instance is being referred to otherwise (well, aside from the WP_Widget::_set( $number ) method which sets WP_Widget::$number, but these are only used in the underlying plumbing and not in the actual API that developers work with.

#3 follow-up: @welcher
9 years ago

  • Keywords dev-feedback added

I think abstracting the code can start with WP_Widget itself. The widget() method is extremely generic and I would start by having it call three new public methods:

  1. before_widget()
  2. widget_markup()
  3. after_widget()

before_widget:

This method would output a standard before widget and title and would be modelled after what we are doing in the majority of the default widgets. i.e:

public function before_widget( $args, $instance ) {
    $title = apply_filters( 'widget_title', ! isset($instance['title']  ) || empty( $instance['title'] ) ? '' : $instance['title'], $instance, $this->id_base );
    echo $args['before_widget'];
    if ( $title ) {
        echo $args['before_title'] . $title . $args['after_title'];
    }
}

after_widget()
Again this would be modelled after what is being done with current default widgets. i.e

public function after_widget( $args, $instance ) {
    echo $args['after_widget'];
}

widget_markup()
This method would handle the custom portion of the widget. We would need some kind of default markup. In a sub-class this would/should contain calls to other methods.

The 'new' widget() method

The current widget method is required to be overridden. I would rather see the widget() method be something like this.

public function widget( $args, $instance ) {
    $this->before_widget( $args, $instance );
    $this->widget_markup();
    $this->after_widget( $args, $instance );
}

This would be backwards compatible the method can still be overridden but any new widgets can take advantage of customizing only the parts they want.

Example using WP_Widget_Search:

/**
 * Search widget class
 *
 * @since 2.8.0
 */
class WP_Widget_Search extends WP_Widget {

	public function __construct() {
		$widget_ops = array('classname' => 'widget_search', 'description' => __( "A search form for your site.") );
		parent::__construct( 'search', _x( 'Search', 'Search widget' ), $widget_ops );
	}

	public function widget_markup() {
		get_search_form();
	}

	public function form( $instance ) {
		$instance = wp_parse_args( (array) $instance, array( 'title' => '') );
		$title = $instance['title'];
?>
		<p><label for="<?php echo $this->get_field_id('title'); ?>"><?php _e('Title:'); ?> <input class="widefat" id="<?php echo $this->get_field_id('title'); ?>" name="<?php echo $this->get_field_name('title'); ?>" type="text" value="<?php echo esc_attr($title); ?>" /></label></p>
<?php
	}

	public function update( $new_instance, $old_instance ) {
		$instance = $old_instance;
		$new_instance = wp_parse_args((array) $new_instance, array( 'title' => ''));
		$instance['title'] = strip_tags($new_instance['title']);
		return $instance;
	}

}

Clearly, this needs some more thought but it's a place to start at any rate.

Last edited 9 years ago by welcher (previous) (diff)

#4 in reply to: ↑ 2 @jacobsantos
9 years ago

  • Keywords dev-feedback removed

Replying to westonruter:

Perhaps the biggest thing that bugs me about WP_Widget is that one instance of this object does not correspond to one instance in the database...

A widget shouldn't care about database details. It should only care about output and configuration or details that concerns that widget implementation and not something that is external to its concern.

The other problem is that the WP_Widget class has concerns that should be separate from actual widgets.

There should be an interface that Widgets implement and passed to WP_Widget.

Last edited 9 years ago by jacobsantos (previous) (diff)

#5 @jacobsantos
9 years ago

  • Keywords dev-feedback added

Oops.

@welcher
9 years ago

@welcher
9 years ago

Corrected diff

#6 @welcher
9 years ago

Added corrected patch for the approach outlined and addresses some standards issues.

#7 in reply to: ↑ 1 @welcher
9 years ago

Replying to jacobsantos:

"Do you even OOP, brah?"

I would hazard to guess that for many users of the platform the answer is - what's OOP?

I wonder how many developers have the level of OOP understanding required to hit the ground running with the items on your list. I'm certainly not advocating 'dumbing it down' but WordPress is known for being user friendly for developers who are closer to entry level and some of these concepts are pretty advanced topics.

For the record, I agree with pretty much everything on the list in theory. If we can have an abstraction layer to hide away the complexities and maintain backwards compatibility, I'm all for it.

Just my 2 cents.

#8 in reply to: ↑ 3 ; follow-up: @jdgrimes
9 years ago

Replying to welcher:

I think abstracting the code can start with WP_Widget itself. The widget() method is extremely generic and I would start by having it call three new public methods:

  1. before_widget()
  2. widget_markup()
  3. after_widget()

I've done something similar to this with the base widget class in one of my plugins.

#9 in reply to: ↑ 8 ; follow-up: @welcher
9 years ago

Replying to jdgrimes:

I've done something similar to this with the base widget class in one of my plugins.

Have you run into any situations where that is limiting?

I like the idea of building out WP_Widget so that child classes contain the smallest amount of code needed - essentially just the changes/new functionality - but if the user is constantly having to override 3 smaller methods instead of one large one, we're not really that much further ahead.

Thoughts?

#10 in reply to: ↑ 9 ; follow-up: @jdgrimes
9 years ago

Replying to welcher:

Replying to jdgrimes:

I've done something similar to this with the base widget class in one of my plugins.

Have you run into any situations where that is limiting?

I like the idea of building out WP_Widget so that child classes contain the smallest amount of code needed - essentially just the changes/new functionality - but if the user is constantly having to override 3 smaller methods instead of one large one, we're not really that much further ahead.

Thoughts?

I have just implemented this recently, but so far I have had no reason to override anything but the widget_markup() method (though in this case it is called widget_body()). I think that probably in 90% of cases that's the only one that would need to be overridden. But if the user needed to, they could always just override the widget() method as a whole in the traditional manner.

If you'd like to take a closer look at what I've done, here is a link to the plugin's main widget class, and another to the code for the plugin's widgets.

#11 in reply to: ↑ 10 ; follow-up: @welcher
9 years ago

Replying to jdgrimes:

I think that probably in 90% of cases that's the only one that would need to be overridden. But if the user needed to, they could always just override the widget() method as a whole in the traditional manner.

That makes sense and aligns with my experiences using a similar approach. I'd love to get a working version of this as a patch and get some more eyes on it.

#12 in reply to: ↑ 11 ; follow-up: @jdgrimes
9 years ago

Replying to welcher:

Replying to jdgrimes:

I think that probably in 90% of cases that's the only one that would need to be overridden. But if the user needed to, they could always just override the widget() method as a whole in the traditional manner.

That makes sense and aligns with my experiences using a similar approach. I'd love to get a working version of this as a patch and get some more eyes on it.

A test of the usefulness of the bootstrap would be to apply it to the core widgets and see how much duplication is removed. Working on that might also show other parts that could be abstracted.

Also, IMO, the patch is hard to follow because there are so many extraneous whitespace changes in it. Maybe you could strip it down to just the functional code changes, and it would be easier to work with?

#13 in reply to: ↑ 12 @welcher
9 years ago

Replying to jdgrimes:

Replying to welcher:

A test of the usefulness of the bootstrap would be to apply it to the core widgets and see how much duplication is removed. Working on that might also show other parts that could be abstracted.

That makes the most sense to me - I'll start on that and get a patch going.

Also, IMO, the patch is hard to follow because there are so many extraneous whitespace changes in it. Maybe you could strip it down to just the functional code changes, and it would be easier to work with?

Absolutely agree - I'll do a new patch with just the functional changes.

thanks!

@welcher
9 years ago

Functional changes to WP_Widget and using new methods in default-widget.php

#14 follow-up: @welcher
9 years ago

  • Keywords has-patch added

New patch in place that better demonstrates the changes proposed to WP_Widget

I have done a first pass on the default widgets and this new approach removes a fair amount of duplication.

Some of the widgets will need slight refactoring to accommodate using the new methods and I have left them as is for now.

A few takeaways:

  1. We need a way of stopping the widget from rendering if there is no content without having to override the whole widget() method
  2. I think we should abstract out the processing for the title parameter. Currently it is in the before_widget() method and it would be better to have access to it where ever needed - which will help the refactoring I mentioned earlier.
  3. We have not accounted for caching - but that might be better suited as a full override of widget() as it won't be needed on every Widget

Any comments/suggestions/notes greatly appreciated!

#15 in reply to: ↑ 14 @jdgrimes
9 years ago

Replying to welcher:

New patch in place that better demonstrates the changes proposed to WP_Widget

I have done a first pass on the default widgets and this new approach removes a fair amount of duplication.

Some of the widgets will need slight refactoring to accommodate using the new methods and I have left them as is for now.

A few takeaways:

  1. We need a way of stopping the widget from rendering if there is no content without having to override the whole widget() method

I think we need to look at the reasons that no content will be displayed and see if there is a common pattern. In several of the cases it is because the settings aren't actually valid. In my plugin I use a verify_settings() method to check that the settings are valid before the widget is displayed. If they aren't, it shows a helpful error message instead (but only if the user has the caps necessary to edit the widget).

  1. I think we should abstract out the processing for the title parameter. Currently it is in the before_widget() method and it would be better to have access to it where ever needed - which will help the refactoring I mentioned earlier.

This sounds like a good idea. I notice that the tag cloud widget provides a default title if it is empty, for example.

  1. We have not accounted for caching - but that might be better suited as a full override of widget() as it won't be needed on every Widget

I think caching is something that can be abstracted out into the main class. For widgets that use caching, they can set a class property as a flag to enable it, like protected $use_caching = true.

@welcher
9 years ago

Adding verify_settings() and widget_title() methods

#16 @welcher
9 years ago

Patch updated to include a verify_settings method and a widget_title method.

I have been thinking a lot about the __construct method and the process that is currently in-place for creating a new widget. Personally I find it awkward and I think we can abstract that away by introducing a 'setup_widget()` method that returns an array containing the values needed to setup a new Widget.

function widget_setup() {
    return array(
        'id_base'           => 'my-new-widget',
	'name'              => esc_html__( 'WIDGET!' ),
	'widget_options'    => array(
		'classname' => 'my-widget',
		'description' => esc_html__( 'An Instance my new widget 2' )
	),
    );
}

If this method is called inside the __construct method, it can be used to setup everything needed without having to create a __construct() method in the child class that calls parent::__construct.

( Excuse the formatting )

public function __construct( $id_base = '', $name = '', $widget_options = array(), $control_options = array() ) {
    //call the setup method
    if ( $setup = $this->widget_setup() ) {
        $setup = wp_parse_args( 
            $this->widget_setup(), 
            array( 
                'id_base' => '', 
                'name' => '', 
                 'widget_options' => array(), 
                 'control_options' => array() 
             ));
    }

    $id_base = ( $setup ) ? $setup['id_base'] : $id_base;
    $name    = ( $setup ) ? $setup['name'] : $name;
    $widget_options = ( $setup ) ? $setup['widget_options'] : $widget_options;
    $control_options = ( $setup ) ? $setup['name'] : $control_options;


    $this->id_base = empty( $id_base ) ? preg_replace( '/(wp_)?widget_/', '', strtolower( get_class( $this ) ) ) : strtolower( $id_base );
    $this->name = $name;
    $this->option_name = 'widget_' . $this->id_base;
    $this->widget_options = wp_parse_args( $widget_options, array( 'classname' => $this->option_name ) );
    $this->control_options = wp_parse_args( $control_options, array( 'id_base' => $this->id_base ) );
}

I wanted to get some opinions/feedback on this before adding it to the patch.

Thanks!

@jacobsantos
9 years ago

32470.4.diff using interfaces

#17 follow-up: @jacobsantos
9 years ago

The interfaces patch introduces interfaces in a naive implementation. This is for feedback and to show what I'm leaning towards. Future patches will hopefully have a better and more complete implementation. Also move default-widgets.php over to the interfaces as well.

The patch currently does not show how to remove the need of extending WP_Widget, but future patch will. I decided also while I was looking over the code that it might be better to just clean up more of the code and move over to a registry and move everything else to interfaces as well. It will still be naive, but I want to write unit tests and get the implementation working on current systems to prove that backwards compatibility can still be maintained, even with going towards interfaces.

The system also needs to review all code with widgets and create a more complete and better implementation with full knowledge of how the system is currently being used. This should also include how plugins and themes are using the API and how the API could be improved to simplify common use cases.

Please review and give feedback. Is it worth continuing or do you have any questions? It might make more sense once the refactoring is complete.

#18 in reply to: ↑ 17 ; follow-ups: @jdgrimes
9 years ago

Replying to jacobsantos:

I have to admit that I'm not familiar with using interfaces, so I'm not sure what the benefits are here. Maybe you could explain? To me it just looks like added layers of complexity without actually reducing the bootstrap code plugin devs have to write.

I think maybe what you envision and what @welcher was proposing are two different things. I think you're leaning towards a complete rewrite of the whole widgets API, while (I think) what @welcher was proposing was just a little bit more abstraction/bootstrap in the WP_Widget class. I think we need to be more specific about the scope of this ticket. I'm not saying your scope is necessarily too broad, but I'm not sure that's what the original intent was, and maybe these are sort of two different things that should be separate tickets. ??

#19 in reply to: ↑ 18 @jacobsantos
9 years ago

Replying to jdgrimes:

Replying to jacobsantos:

I have to admit that I'm not familiar with using interfaces, so I'm not sure what the benefits are here. Maybe you could explain? To me it just looks like added layers of complexity without actually reducing the bootstrap code plugin devs have to write.

Right. So I think it will become more clear in a later patch when I actually use the interfaces as opposed to just abstracting out some of the existing code.

I'm not sure what layers of complexity would be added. Can you explain a little bit more what you don't understand? If it is just simply that you don't think adding implements is useful, then yes, that should be more clear in a later patch. I think that I'm going to remove some of the changes to WP_Widget and just implement the correct interfaces. I am going to still move some of the code out of the WP_Widget, but it will be less of a concern than methods that could be replaced.

I'm going to create a new object that is going to just proxy or adapter the implementation instead. This implementation will have what I wanted to do with WP_Widget and remove the need to change WP_Widget except for a few utility functions that will be moved to a separate class.

As for reducing the amount of code, that is something I will address. I'm still attempting to comprehend the scope of the widgets and I'm still breaking out the various parts of the WP_Widget class into discrete parts. For the most part, I think the only change the default widgets will have is that they won't extend WP_Widget, they will just implement WP_Widget_ControlInterface, WP_Widget_SettingInterface, WP_Widget_Display_EventInterface and maybe WP_Widget_AdminInterface. If it seems like every widget has majority of the methods, then I might combine some interfaces. I just like to have small interfaces and combining them into larger interfaces rather than have large interfaces and try to break them out.

The goal is to see where common code exists and reuse interfaces wherever possible, rather than have over broad interfaces.

The second purpose and far better reason to have interfaces is creating unit tests. Interfaces allow for using dummies, fakes, stubs, and mocks. By having smaller interfaces, you can create different test objects that have specific purposes. So I can create a dummy that implements WP_Widget_AdminInterface that doesn't do anything. Or create fakes that don't write to the database, but store the changes where they can be tested later.

So while there is going to be quite a bit more code, the advantages will be better tests. Also, the advantages will become far more apparent later when plugin or theme developers start using the new interfaces. I do fear that the knowledge base would perhaps add more frustration than benefit to those who don't know how to properly use interfaces. I think those who are more experienced will have an extremely fun time.

I think ultimately, the final implementation is going to add a lot more code, but will also allow for much better unit and integration testing. I also hope to change the system testing for widgets to better take advantage of the changes.

The entire refactoring has to be taken together, I guess, and I hope you understand once I have a more complete implementation.

I think maybe what you envision and what @welcher was proposing are two different things. I think you're leaning towards a complete rewrite of the whole widgets API, while (I think) what @welcher was proposing was just a little bit more abstraction/bootstrap in the WP_Widget class. I think we need to be more specific about the scope of this ticket. I'm not saying your scope is necessarily too broad, but I'm not sure that's what the original intent was, and maybe these are sort of two different things that should be separate tickets. ??

The scopes are definitely different, but both fit into this ticket. I think certain parts of my patch could still be applied to this ticket as a base for future tickets. My scope is that, improving WP_Widget should extend beyond simply adding more methods. It should extend to better tests and improved functionality. It is something that I wished I had done when WP_Widget was first introduced. It has always been a flawed implementation and my goal is to improve it to where it hopefully should have been when it was first introduced.

Again, not so much a rewrite, because all of the existing code will still be part of the implementation. If you do refactoring correctly, then you don't rewrite something as move existing parts around to create something that is hopefully better.

If it is the case that the patch scope is too broad, then I will create a new ticket. I think it should still be possible to do what I need to do without the code in this ticket's patch. I will still include it to show the purpose of the changes.

#20 @jacobsantos
9 years ago

So the goal is to introduce interface segregation, single responsibility and dependency inversion. Doing so to split out the god object WP_Widget into separate classes that focus on a single concern. Part of that has to do with creating interfaces for both splitting into single concerns as well as help with dependency inversion.

It is my prediction that if WordPress includes better OOP principles in the code, that programmers will become better object-oriented programmers. I'm not sure whether this claim is falsifiable, given that it is inherent in the practices, knowledge and experience of individual programmers. Some may embrace better OOP principles and become better programmers, but it is unknowable, except through informal polls, whether it was because of WordPress or they simply adapted it on their own.

At the very least, the hope is that core contributors and committers adopt better OOP practices and improve the code to allow for more comprehensive and better test suite than what is available at the time of this writing. Very little of WordPress can be unit tested and forget integration tests with much of the existing code base. At best, system test cases can be created for majority of the code base with some opportunities for unit and integration test cases.

My goal is to show what better OOP could allow and attempt to convince as many contributors and committers as possible that it is in fact possible in the code base without breaking backwards compatibility.

This ticket was mentioned in Slack in #core-multisite by jacobsantos. View the logs.


9 years ago

#22 in reply to: ↑ 18 @welcher
9 years ago

Replying to jdgrimes:

Replying to jacobsantos:

To me it just looks like added layers of complexity without actually reducing the bootstrap code plugin devs have to write.

I agree, the interface approach ( at least in the patch provided ) is very complicated and I don't immediately see the benefit of requiring a new class to implement 4 interfaces rather that extend a single class. As I mentioned in a earlier comment, WordPress maintains a low barrier-to-entry for developers and for some, doing any OOP may be daunting. I am guessing this is why WP_Widget was written to be inherited. There seems to be a lot of room for error with having to implement multiple interfaces. If one is left out, what happens? The point of an interface is to define what methods a class must define, and if that is what where trying to accomplish, it may be a better approach to make WP_Widget an abstract class with abstract methods and maintain the inheritance model.

I think maybe what you envision and what @welcher was proposing are two different things. I think you're leaning towards a complete rewrite of the whole widgets API, while (I think) what @welcher was proposing was just a little bit more abstraction/bootstrap in the WP_Widget class.I think we need to be more specific about the scope of this ticket. I'm not saying your scope is necessarily too broad, but I'm not sure that's what the original intent was, and maybe these are sort of two different things that should be separate tickets. ??

That is what I was proposing - more abstraction in WP_Widget. I think that perhaps a second ticket is in order as @jacobsantos approach would require a major rewrite to the API and is beyond the scope of making enhancements to what is currently in place.

#23 @jacobsantos
9 years ago

I made a video further explaining the motivations and reasons behind the changes. Not sure if you would sit through an hour video but here you go. https://www.youtube.com/watch?v=ot4LTj04KoE

I think the point that matters the most is the last 3 to 5 minutes. I think that is really what I want to show.

interface WP_DisplayInterface {
        public function render();
}

interface WP_Display_BeforeInterface {
        public function before_render();
}

interface WP_Display_AfterInterface {
        public function after_render();
}

interface WP_Display_VerifyInterface {
        public function should_render();
}

abstract class WP_Abstract_Display
        implements WP_DisplayInterface, WP_Display_VerifyInterface {

        public function render() {
                if ( ! $this->should_render() ) {
                        return;
                }

                if ($this instanceof WP_Display_BeforeInterface) {
                        $this->before_render();
                }

                $this->display();

                if ($this instanceof WP_Display_AfterInterface) {
                        $this->after_render();
                }
        }

        abstract public function should_render();

        abstract function display();
}

The ultimate point is to go towards this code and not the current code. The purpose I'm going to go into is how to develop with reusability in mind. The idea of creating generic interfaces and classes that can then be further reused in any place.

What exactly is a Widget? An object that displays arbitrary content at a given location. Okay. Fine. Why does it need to have a specialized contract, when a generic one would satisfy? I think the problem is that the developers didn't think generally but specifically towards solving the problem. Well, then again, the code needed to support PHP4, which didn't support interfaces, so that is part of the problem.

There is another problem that the code would be better off using namespaces to better segment and name the code. The solution should be solved with as minimal amount of code as possible. That is also ultimately what I'm attempting to show.

The above solves your problem without adding any new methods.

class WP_Widget_Control extends WP_Widget {
        protected $display_instance = null;

        public function __construct(WP_DisplayInterface $display) {
                // Call parent::__construct();
                $this->display_instance = $display;
        }

        public function widget( $args, $instance ) {
                $this->display_instance->render();
        }
}

The below code also implements a solution to your problem, again without adding any more methods to the existing class. Furthermore, the new interfaces could be used within WP_Widget with an test as to whether the interface is implemented and then called.

final class WP_Widget_Display_Adapter implements WP_DisplayInterface {

        private $_storage = array(
                'display' => null,
                'before' => null,
                'after' => null,
                'verify' => null,
        );

        public function register($instance) {
                switch (true) {
                        case $instance instanceof WP_DisplayInterface:
                                $this->_storage['display'] = $instance;
                        break;
                        case $instance instanceof WP_Display_BeforeInterface:
                                $this->_storage['before'] = $instance;
                        break;
                        case $instance instanceof WP_Display_AfterInterface:
                                $this->_storage['after'] = $instance;
                        break;
                        case $instance instanceof WP_Display_VerifyInterface:
                                $this->_storage['verify'] = $instance;
                        break;
                }
                return $this;
        }

        public static function factory() {
                if ( func_num_args() < 1 ) {
                        return new static;
                }

                $instance = new static;

                $args = func_get_args();

                foreach ($args as $obj) {
                        $instance->register($obj);
                }
                return $instance;
        }

        public function render() {
                if ( is_object( $this->_storage['verify'] ) && ! $this->_storage['verify']->should_render() )
                        return;
                }

                if ( is_object( $this->_storage['before'] ) ) {
                        $this->_storage['before']->before_render();
                }

                if ( is_object( $this->_storage['display'] ) ) {
                        $this->_storage['display']->render();
                }

                if ( is_object( $this->_storage['after']) ) {
                        $this->_storage['after']->after_render();
                }
        }

}

I'm not sure how to explain what I mean any better. Which part do you not understand? I think the problem with the patch is that I tried to show how the code could be refactored and I think I'm going away from that goal and towards what I should have attempted to explain previously. This is my end goal. This is where I want the code to go towards. I would have done it in a roundabout way, but at least you get to see where the finish line is located.

True, it doesn't exactly fit in with the goals you are attempting to design. However, in all honestly, your patch and design should not be allowed as it only compounds the existing problem. The original implementation should not have been allowed, if but for compat with PHP4.

This ticket was mentioned in Slack in #core-multisite by jacobsantos. View the logs.


9 years ago

#25 follow-up: @jdgrimes
9 years ago

Thanks for the explanation @jacobsantos. I'm starting to see better where you are going with this. I can grasp how this will improve the testability of the API. And I can also see now that you're not actually trying to rewrite the entire API, just to refactor WP_Widget. Ultimately it is the same thing as @welcher proposed in terms of scope, but you are reducing the complexity of the WP_Widget class itself, rather than increasing it. This actually gives us more freedom to reduce duplication across the widgets because we won't be tied to having just one all-encompassing abstraction in WP_Widget, we can have several different flavors of widget that implement these interfaces.

It is my prediction that if WordPress includes better OOP principles in the code, that programmers will become better object-oriented programmers.

As someone who has learned PHP practices mostly from WordPress, I can say that the way I do OOP is largely based on what WordPress does. If WordPress improves, so will I, and many others will too. However, there is a difference between growing my knowledge of OOP alongside WordPress and a newbie trying to break into WordPress development after the fact. I think if it *seems* too complex, it may discourage some people from getting in (not necessarily a bad thing, mind you). It took both me and @welcher some time to wrap our minds around it, but I think in some ways the end product will actually be simpler than what is was before, once a dev cracks the entry barrier. And, because backward compatibility will be maintained, newbies can continue to use the old approach until they better understand the new underlying API.


Note: I see that you are using static in WP_Widget_Display_Adapter::factory(), however, WordPress still supports PHP 5.2 (not my fault :-).

Last edited 9 years ago by jdgrimes (previous) (diff)

#26 in reply to: ↑ 25 @jacobsantos
9 years ago

Replying to jdgrimes:

Thanks for the explanation @jacobsantos. I'm starting to see better where you are going with this. I can grasp how this will improve the testability of the API. And I can also see now that you're not actually trying to rewrite the entire API, just to refactor WP_Widget.

Well, my next patch will go into refactoring more of the widgets.php file. I think I will create a new ticket for that as it is outside the scope of WP_Widget. I think there are other areas in WP_Widget that could be further refactored. I'm unsure how to proceed with this ticket. Given its scope, I was thinking about using the interfaces and testing for it, sort of how I shown in my previous example.

Ultimately it is the same thing as @welcher proposed in terms of scope, but you are reducing the complexity of the WP_Widget class itself, rather than increasing it. This actually gives us more freedom to reduce duplication across the widgets because we won't be tied to having just one all-encompassing abstraction in WP_Widget, we can have several different flavors of widget that implement these interfaces.

That is part of my goal. There are two others. The second is to reduce code within WP_Widget and other parts of widgets.php by further abstracting and refactoring the code. The final is to go in and allow for certain parts of the existing code base to be replaced. I think I put another hour or two into it last night and I'm barely there. I thought this would take a day (8 hours) and it still might, but it will be over the next couple of days.

The difficulty is finding a pattern that exists in two or more places and creating an interface that matches that pattern. I fear that barring a simple low modification patch, it won't be accepted into core. What I'm doing is on the level of WP Meta Field changes, which might require far more red tape. Granted, I'm going to tackle a few other tickets and apply the same techniques to those as well. Perhaps more patterns will emerge that will further reduce the amount of code required.

It is my prediction that if WordPress includes better OOP principles in the code, that programmers will become better object-oriented programmers.

As someone who has learned PHP practices mostly from WordPress, I can say that the way I do OOP is largely based on what WordPress does. If WordPress improves, so will I, and many others will too.

I think unfortunately, that WordPress is the level most programmers stop, even outside of WordPress. It takes something like Laravel to go further and see the full power of using interfaces. I think Laravel framework is a good example, because it strives to be as simple as possible, while still giving as much power as possible. I hope this is possible in WordPress, so not only would novices have an easier time, the flow of their application would allow them to further their knowledge and experience as they continue developing with WordPress.

Unfortunately, some of that power has to wait until PHP5.3. It is incredible how using a few generalized interfaces can change the application and the patterns it allows.

However, there is a difference between growing my knowledge of OOP alongside WordPress and a newbie trying to break into WordPress development after the fact. I think if it *seems* too complex, it may discourage some people from getting in (not necessarily a bad thing, mind you). It took both me and @welcher some time to wrap our minds around it, but I think in some ways the end product will actually be simpler than what is was before, once a dev cracks the entry barrier. And, because backward compatibility will be maintained, newbies can continue to use the old approach until they better understand the new underlying API.

The question I have is the reasons behind that and how to allow for it to continue while still creating better code. My prediction is that contributors and committers won't really consider any previous design decisions and will develop their patch at their level regardless. I see that with the current stream of patches and contributions. Well, some of it might simply be preparing refactoring and may eventually abstract further, so I might be too critical at this point.

I have experienced that as well. I think the problem is the burden of design. With WordPress there is no burden of design, because WordPress does not suffer from having a design and designer. It is a mash of code that works together. It is the hacker's dream, because everything has been hacked in. Nothing was considered from the perspective of the original author.

As much as I would like to hope that composition would enter the mindset of plugin and theme developers, along with contributors. I don't believe it will for all. Part of the noise I'm creating is the futile attempt at saying, "Hey hey. Let's design something better." I hope that it might be better this round, but past experience has taught me that I'm better off working with my github fork.

I think that part of it is education. Something I regret and will have to see whether it makes a difference is that I never documented the reasons for why I decided to do what I did when I did them. Something I'm attempting to do with the videos and something that needs to be added to the handbook. Provided any of my patches are actually committed to core.

Being able to compose functions is, I think, the next step with clean code. Being able to compose objects along with functions is something I'm still experimenting with.

Note: I see that you are using static in WP_Widget_Display_Adapter::factory(), however, WordPress still supports PHP 5.2 (not my fault :-).

It was example code. I didn't want to do new WP_Widget_Display_Adapter, because I don't normally develop as far back as support PHP 5.2. Not being able to use closures would be a nightmare.

#27 follow-up: @Frank Klein
9 years ago

The proposed patch by @jacobsantos seems interesting, but it also is a lot of code movement. This means that it probably won't make it into a release very fast, as a lot of testing would have to be done.

Also when working on new abstractions, we need to keep in mind which classes and methods are currently public. We would need to ensure that some form of backwards compatibility is maintained, because currently nothing keeps developers from directly accessing this code in their projects.

In the meantime, I think we could have tangible benefits by doing smaller changes, like for example:

  • Making WP_Widget an abstract class.
  • Marking the methods that need to be overriden as abstract.
  • Marking functions that should not be overriden as final.
  • Marking private functions as private.

Like that we could progress while working on a larger refactoring.

Why not keep this ticket around for the larger overall planning, and have a ticket with a smaller scope to go along with #23012, focussing on improvements to WP_Widgets and WP_Widget_Factory that can be done right now.

#28 in reply to: ↑ 27 ; follow-up: @jdgrimes
9 years ago

Replying to Frank Klein:

The proposed patch by @jacobsantos seems interesting, but it also is a lot of code movement. This means that it probably won't make it into a release very fast, as a lot of testing would have to be done.

I think @jacobsantos intends to maintain full backward compatibility. And I think that he's also committed to writing unit tests and integration tests, since that is part of the reason he is proposing these changes. But yes, it will still certainly need lots of testing.

Also when working on new abstractions, we need to keep in mind which classes and methods are currently public. We would need to ensure that some form of backwards compatibility is maintained, because currently nothing keeps developers from directly accessing this code in their projects.

In the meantime, I think we could have tangible benefits by doing smaller changes, like for example:

  • Making WP_Widget an abstract class.
  • Marking the methods that need to be overriden as abstract.
  • Marking functions that should not be overriden as final.
  • Marking private functions as private.

Like that we could progress while working on a larger refactoring.

Why not keep this ticket around for the larger overall planning, and have a ticket with a smaller scope to go along with #23012, focussing on improvements to WP_Widgets and WP_Widget_Factory that can be done right now.

None of those changes are backward compatible (though for some we could employ hacks to make them more so). I think when this larger refactoring is complete, most of those changes won't matter anyway, because many of the old methods will probably only be there for back-compat anyway. So it might not be worth the time to do most of them. There might be other incremental improvements that could be made, though.

Last edited 9 years ago by jdgrimes (previous) (diff)

#29 in reply to: ↑ 28 @jacobsantos
9 years ago

I was informed by nacin on slack (do I need a link, I probably do) that none of the proposals I have submitted will be accepted. I am guessing this includes the architectural changes included in the prototype patch I submitted in this ticket. I am also guessing that it is irrelevant whether or not unit and integration tests are included or if the current system tests are extended.

I will continue my work on my fork and hopefully at some point down the road it will be included. At least it will be a better basis of what I'm attempting to describe and hopefully prove that such refactorings are possible without breakage and prove that refactorings allow for better code and less bugs at the same time.

Or I might just discover that I was wrong. It is a possibly that I am wrong, but I do have enough anecdotal evidence to suggest that what I proposed is the way to go for better tests and maintainability.

I am not even certain if it is just because I proposed the changes or if you would even be allowed to implement what I propose. If you can take what I wrote and apply it to your changes, then perhaps it might serve as a launching point for improvements into WordPress. I think the caveat, is that if the architectural improvements have a good reason for being implemented, it could be accepted.

My goal and changes are out-of-scope with this new information. Do what you will, I will not be part of this discussion from this point. Given the amount of work I have already put into refactoring WP_Widget and everything, I am guessing that I will be working on it soon and if you would like to know more about what I proposed I can send you a link to the experimental library.

Replying to jdgrimes:

Replying to Frank Klein:

The proposed patch by @jacobsantos seems interesting, but it also is a lot of code movement. This means that it probably won't make it into a release very fast, as a lot of testing would have to be done.

I think @jacobsantos intends to maintain full backward compatibility. And I think that he's also committed to writing unit tests and integration tests, since that is part of the reason he is proposing these changes. But yes, it will still certainly need lots of testing.

Also when working on new abstractions, we need to keep in mind which classes and methods are currently public. We would need to ensure that some form of backwards compatibility is maintained, because currently nothing keeps developers from directly accessing this code in their projects.

In the meantime, I think we could have tangible benefits by doing smaller changes, like for example:

  • Making WP_Widget an abstract class.
  • Marking the methods that need to be overriden as abstract.
  • Marking functions that should not be overriden as final.
  • Marking private functions as private.

Like that we could progress while working on a larger refactoring.

Why not keep this ticket around for the larger overall planning, and have a ticket with a smaller scope to go along with #23012, focussing on improvements to WP_Widgets and WP_Widget_Factory that can be done right now.

None of those changes are backward compatible (though for some we could employ hacks to make them more so). I think when this larger refactoring is complete, most of those changes won't matter anyway, because many of the old methods will probably only be there for back-compat anyway. So it might not be worth the time to do most of them. There might be other incremental improvements that could be made, though.

This ticket was mentioned in Slack in #core by welcher. View the logs.


9 years ago

#31 follow-up: @jdgrimes
9 years ago

OK, lets see if we can get this ticket back on track.

The main problem with this ticket is that it has no definite sense of direction. It proposes a "solution" (abstracting the widget classes) without specifying the problems that it is trying to solve. (Yeah, it's easy to do that!) That makes it really easy to end up down a rabbit hole.

A better approach would be to identify specific problems with the current implementation, and then discuss the best way to fix them. Some of them may need to be opened as separate tickets, while some of them may have common solutions that would fall under this ticket.

Throughout the process we have to keep in mind exactly what widgets are, and what their future is. We don't want to make changes that will tie the widgets API down to something just when widgets might be evolving to be something else.

It may sound like I'm blowing this way out of proportion, but I think if we really want the core devs to ever consider any changes of this scope to the widgets API, we need to take this approach. At this stage, we haven't considered what widgets are or what they will be tomorrow, and we haven't identified what problems we are trying to solve or why this will be a lasting solution. It is really too early for a ticket at all. If we're serious about this, I think we should start/join a widgets working group. Any thoughts?

#32 in reply to: ↑ 31 @welcher
9 years ago

Replying to jdgrimes:

I agree 100% that this ticket has fallen off the rails a bit.

I have done some thinking on a roadmap/approach and in order to affect any change to the Widgets API we need make small iterative changes while moving towards the goal of a better Widget API/experience.

In my opinion, the first step is to look at what we have and make any changes or fixes there.

default-widgets.php

  1. Continue/finish the code audit in #23012 to bring the default widgets in-line with WP standards/best practices
  2. Review and if needed, update the inline docs for the default widget. I know there are some areas of improvement for example WP_Widget_RSS has very little in the way of inline docs. This is going to provide a lot of insight into what might need to be changed. - Related #30315
  3. Identifying functionality in the default widgets that can be encapsulated into methods. This will allow for much easier extension into child class. Currently, extending a default widget to override even a small change in the functionality usually requires a copy/pasta of the majority of the code.

WP_Widget/General API class:

This is where the rabbit hole begins, as there are many things we can do here. As a starting point, we should look at small improvements most of which have been discussed in this ticket already.

  1. Further abstraction for widget() to contain calls to other methods such as before_widget(), after_widget() and widget_markup(). This will allow for less code in child classes as they will only need to override what needs to be changed.
  2. Find a better/alternative to the current usage of __construct in child classes. IMHO, this is needlessly complicated
  3. Address the PHP4 constructor issue - #32480.

The items above would get Widgets into a much better place and gives us a roadmap with actual items to check off as we move ahead.

As usual, I'd love any feedback or suggestions!

Last edited 9 years ago by welcher (previous) (diff)

#33 @welcher
9 years ago

  • Keywords 2nd-opinion added; has-patch removed

#36 @welcher
6 years ago

  • Keywords bulk-reopened added
  • Resolution set to invalid
  • Status changed from new to closed

Based on where Gutenberg is headed in Phase 2. This ticket is no longer valid.

Note: See TracTickets for help on using tickets.