Make WordPress Core

Opened 10 years ago

Closed 8 years ago

Last modified 8 years ago

#28216 closed defect (bug) (fixed)

Allow to register pre-instantiated widgets

Reported by: denis-de-bernardy's profile Denis-de-Bernardy Owned by: westonruter's profile westonruter
Milestone: 4.6 Priority: normal
Severity: normal Version: 3.9.1
Component: Widgets Keywords: has-patch
Focuses: Cc:

Description

As things stand, the current API doesn't allow to instantiate a widget that relies on dependency injection because register_widget() and WP_Widget_Factory#register() expect a class name as a parameter.

It would be convenient if the factory would conditionally instantiate the widget classes instead:

	function register($widget_class) {
		$this->widgets[$widget_class] = is_object($widget_class) ? $widget_class : new $widget_class();
	}

Attachments (8)

28216.exit-early-implementation.patch (2.2 KB) - added by mdwheele 9 years ago.
Implementation that exits early.
28216.branching-implementation.patch (2.2 KB) - added by mdwheele 9 years ago.
Implementation that branches.
28216.update-api-function-docs.patch (1.3 KB) - added by mdwheele 9 years ago.
Updates register_widget and unregister_widget docs to reflect change in argument type expectation.
28216.spl_object_hash-approach.patch (2.2 KB) - added by PeterRKnight 8 years ago.
mdwheele's patch using spl_object_hash instead of get_class allowing for multiple instances of class
28216.5.diff (4.4 KB) - added by westonruter 8 years ago.
28216.6.diff (4.4 KB) - added by westonruter 8 years ago.
Fix docs
28216.7.diff (5.5 KB) - added by westonruter 8 years ago.
Test registering multiple instances of the same class
28216.8.fallback.diff (5.2 KB) - added by westonruter 8 years ago.
Provide fallback if spl_object_hash() is not available

Download all attachments as: .zip

Change History (37)

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


9 years ago

#2 @valendesigns
9 years ago

  • Keywords reporter-feedback added

@Denis-de-Bernardy could you please expand on why this functionality is needed from your perspective? I'm not sure I clearly see why Core should be supporting an alternative method for programmatically adding widgets. As well, if you could attach a code example. Any details you could provide here would be a huge help if you want to see any movement on this issue.

#3 @Denis-de-Bernardy
9 years ago

Sure. If you want your widgets to use a dependency injection, they need to be instantiated like so:

$my_widget = new MyWidget($dep1, $dep2);
wp_register_widget($my_widget); // or perhaps more appropriately, $my_widget->register();

... but as things stand, WP expects a call more like:

wp_register_widget('MyWidget'); // and now $dep1 and $dep2 dependencies are missing.

To work around the issue, you currently need to edit the widget factory's private variables.

#4 @Denis-de-Bernardy
9 years ago

  • Keywords reporter-feedback removed

#5 @valendesigns
9 years ago

Thank you for responding back. Could you please be more specific about what use case dependency injection has and why core should support it?

#6 follow-up: @Denis-de-Bernardy
9 years ago

The use-case for dependency injection is probably too broad for a Trac answer. In a nutshell, though, it's about composition, loose coupling, and testability. A URL for further reading:

http://stackoverflow.com/q/130794/417194

In the event it needs to be stressed, the purpose to be play better with PHP frameworks such as Symfony 2:

http://symfony.com/doc/current/components/dependency_injection/index.html

I was looking into creating a sensible WP Widget wrapper that played well with a Symfony DIC when I opened this ticket.

#7 in reply to: ↑ 6 ; follow-up: @westonruter
9 years ago

Replying to Denis-de-Bernardy:

The use-case for dependency injection is probably too broad for a Trac answer. In a nutshell, though, it's about composition, loose coupling, and testability. A URL for further reading:

http://stackoverflow.com/q/130794/417194

In the event it needs to be stressed, the purpose to be play better with PHP frameworks such as Symfony 2:

http://symfony.com/doc/current/components/dependency_injection/index.html

I was looking into creating a sensible WP Widget wrapper that played well with a Symfony DIC when I opened this ticket.

Why not use setter injection or property injection instead of constructor injection? For instance:

<?php
add_action( 'widgets_init', function () {
    /** @var \WP_Widget_Factory $wp_widget_factory */
    global $wp_widget_factory;
    register_widget( 'My_Widget' );
    $wp_widget_factory->widgets['My_Widget']->otherDependency = $foo;
} );

#8 in reply to: ↑ 7 ; follow-up: @Denis-de-Bernardy
9 years ago

Replying to westonruter:

Why not use setter injection or property injection instead of constructor injection?

Because there's something wrong with engineering software like that in my opinion except in very special cases.

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

Replying to Denis-de-Bernardy:

Because there's something wrong with engineering software like that in my opinion except in very special cases.

OK, I don't have any problem with the proposed change. Can you put together a patch, including unit tests?

#10 in reply to: ↑ 9 @mdwheele
9 years ago

Replying to westonruter:

OK, I don't have any problem with the proposed change. Can you put together a patch, including unit tests?

If it's not against contribution standards for me to do so, I can prepare a patch for this feature. I found this ticket in the process of looking to fix the same issue myself.

To expand upon the use-case for this being made-available to core, I have an example:

I maintain a set of microservices where I work that are responsible for serving HR and Student data as well as other use-cases. For this particular use-case, we have a service responsible for providing query capabilities for a University job-board. This is exposed as a JSON API and I have also written packages around that API for PHP-consumption. All this leads to... the implementation of a client that talks to that API conforms to a known interface and I want to make a WordPress widget that depends on that client interface to query the job board. I maintain a package written in PHP for querying this API to be used by web applications and other PHP clients. Taking advantage of this package in a WordPress widget or plugin should be a simple matter of wiring-only. Not a lot of custom logic and if there IS any, it's simple UI / WordPress-focused branching.

The biggest reason for wanting DI support for things like Widgets is that I can treat the widgets as simple-wiring of my own services into WordPress. I treat Plugins exactly the same; simple-wiring of a domain model into WordPress. Therefore, when I test, my focus is on a WordPress-agnostic domain model. When I write integration tests with WordPress and my custom model, I'm making sure that when a widget is "booted" it calls the right methods on my services. I do not necessarily care to invoke my services as part of that test... I just want to have spies tell me if things aren't called as expected.

This can certainly be achieved through setter-injection, but is more conventional to implement through constructor injection. More importantly, constructor injection FIRMLY declares that X widget wiring has a dependency on some service layer. Setter injection does not do so explicitly. In addition (and probably more importantly, as @Denis-de-Bernardy said), injecting as described above requires a client to intimately understand not-only my Widget and its internal dependencies to be able to inject, but also must intimately be aware of WordPress internals, side-stepping the Core's framework for registration of Widgets. If that ever changed, clients would be broken. It is always preferable for clients to be able to rely on a stable abstraction.

Anywho... I typed all that out just to give a more in-depth use-case than we currently had. Also, I've made the changes to enable this and would GLADLY contribute upstream.. However, this would be my first contribution to core and I wanted to make sure it was cool for a sideline observer to send code for someone elses Trac ticket! :)

Thanks all!

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

@mdwheele
9 years ago

Implementation that exits early.

@mdwheele
9 years ago

Implementation that branches.

@mdwheele
9 years ago

Updates register_widget and unregister_widget docs to reflect change in argument type expectation.

#11 follow-up: @mdwheele
9 years ago

I've added two possible implementations that vary on whether branching or exiting-early is preferred in the widget factory register and unregister methods. This includes a test that has been tagged with this ticket number. Tests are passing. The third patch updates the register_widget and unregister_widget functions to reflect the change in argument type expectation.

I apologize in advance if this oversteps standards for contribution! I wanted to make sure I packaged this up before moving on to other tasks. Please let me know if there's anything you'd like changed!

#12 @mdwheele
9 years ago

Are there any updates on this?

#13 @westonruter
9 years ago

  • Milestone changed from Awaiting Review to Future Release

No update, other than let's look at this in 4.4

#14 @mdwheele
9 years ago

Sounds good. Was just curious and making sure nothing more was needed! Thanks!

#15 @westonruter
8 years ago

  • Owner set to westonruter
  • Status changed from new to accepted

#17 @westonruter
8 years ago

Aside: This is somewhat related to the recent WP-CLI changes to avoid having to register a command by providing a class name but instead to allow passing an object instance (here a callable): https://github.com/wp-cli/wp-cli/issues/2204

#18 @westonruter
8 years ago

  • Milestone changed from Future Release to 4.6

#19 in reply to: ↑ 11 @PeterRKnight
8 years ago

Replying to mdwheele:

I've added two possible implementations that vary on whether branching or exiting-early is preferred in the widget factory register and unregister methods. This includes a test that has been tagged with this ticket number. Tests are passing. The third patch updates the register_widget and unregister_widget functions to reflect the change in argument type expectation.

Your patch allows for pre-initialized widgets but if you try to instantiate multiple widgets of a certain class with arguments, they overwrite each other as far as I can see. So it doesn't cover the example given by @Denis-de-Bernardy here https://core.trac.wordpress.org/ticket/28216?replyto=11#comment:3

I would love to see core support for creating dynamic widgets make it into 4.6!

@PeterRKnight
8 years ago

mdwheele's patch using spl_object_hash instead of get_class allowing for multiple instances of class

#20 follow-up: @westonruter
8 years ago

  • Keywords has-patch added

@PeterRKnight How fortunate that spl_object_hash() is available in PHP 5.2! However, I see it is used in _wp_filter_build_unique_id() but has an alternate case for handling if the function is not defined. This worried me in that perhaps the function could be disabled in 5.2. But see #21267:

Since PHP 5.2 (the required version for WP is 5.2.4), spl_object_hash exists, which means the else statement and function_exists() check are completely useless in _wp_filter_build_unique_id()

So I think we're safe.

@westonruter
8 years ago

#21 @westonruter
8 years ago

  • Keywords commit added

I fleshed out the unit tests, testing the registration of a class name and a class instance together, ensuring both result in registered widgets. See 28216.5.diff. I think this is good to go!

#22 @ocean90
8 years ago

@westonruter Inline @see tags should only be used for hooks.

@westonruter
8 years ago

Fix docs

@westonruter
8 years ago

Test registering multiple instances of the same class

#23 @westonruter
8 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 37329:

Widgets: Allow WP_Widget subclass instances (objects) to be registered/unregistered in addition to WP_Widget subclass names (strings).

Allows widgets to be registered which rely on dependency injection. Also will allow for new widget types to be created dynamically (e.g. a Recent Posts widget for each registered post type).

See #35990.
Props mdwheele, PeterRKnight, westonruter.
Fixes #28216.

#24 in reply to: ↑ 20 @jdgrimes
8 years ago

Replying to westonruter:

@PeterRKnight How fortunate that spl_object_hash() is available in PHP 5.2! However, I see it is used in _wp_filter_build_unique_id() but has an alternate case for handling if the function is not defined. This worried me in that perhaps the function could be disabled in 5.2. But see #21267:

Since PHP 5.2 (the required version for WP is 5.2.4), spl_object_hash exists, which means the else statement and function_exists() check are completely useless in _wp_filter_build_unique_id()

So I think we're safe.

The reason that the function_exists() check is needed is that the SPL extension can be disabled prior to PHP 5.3.0. So in 5.2.0 it is on by default but prior to 5.3.0 it can be disabled. In which case the functions that it provides wouldn't be available.

@westonruter
8 years ago

Provide fallback if spl_object_hash() is not available

#25 follow-up: @westonruter
8 years ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

@jdgrimes thank you for that information. How does 28216.8.fallback.diff look for providing a fallback in the case where spl_object_hash() is not available? It takes a somewhat similar approach as _wp_filter_build_unique_id().

#26 in reply to: ↑ 25 @jdgrimes
8 years ago

Replying to westonruter:

@jdgrimes thank you for that information. How does 28216.8.fallback.diff look for providing a fallback in the case where spl_object_hash() is not available? It takes a somewhat similar approach as _wp_filter_build_unique_id().

That looks OK to me. (Though admittedly I haven't been following this ticked closely, so don't take my word for it. :-)

#27 @westonruter
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 37333:

Widgets: Provide PHP 5.2 fallback for spl_object_hash() if disabled in logic for registering and unregistering pre-instantiated widgets.

Fixes #28216.

#28 @ocean90
8 years ago

  • Keywords needs-dev-note added

#29 @ocean90
8 years ago

  • Keywords needs-dev-note removed
Note: See TracTickets for help on using tickets.