WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#34596 closed enhancement (fixed)

WP_Customize_Manager::add_*() methods should return the added instance

Reported by: westonruter Owned by: westonruter
Milestone: 4.5 Priority: normal
Severity: normal Version: 3.4
Component: Customize Keywords: good-first-bug has-patch has-unit-tests
Focuses: Cc:

Description

A common pattern for adding a panel/section/control/setting to the Customizer and then to access the instance of the thing that was added is:

<?php
$wp_customize->add_control( 'foo', array( /* ... */ ) );
$control = $wp_customize->get_control( 'foo' );
/* Now do what is needed with $control... */

Or sometimes

<?php
$control = new WP_Customize_Control( $wp_customize, 'foo', array( /* ... */ ) );
$control = $wp_customize->add_control( $control );
/* Now do what is needed with $control... */

But it would be ideal if this could be condensed down to:

<?php
$control = $wp_customize->add_control( 'foo', array( /* ... */ ) );
/* Now do what is needed with $control... */

So WP_Customize_Manager::add_control() should return the WP_Customize_Control instead of void.

This same pattern should be done for:

  • WP_Customize_Manager::add_section()
  • WP_Customize_Manager::add_control()
  • WP_Customize_Manager::add_setting()
  • WP_Customize_Manager::add_panel()

Unit tests should be added for each of these methods to ensure that the expected instance is returned in the two patterns of calling the methods, e.g.:

  • WP_Customize_Manager::add_control( $control_id, $args )
  • WP_Customize_Manager::add_control( $control_instance )

Attachments (4)

34596.diff (3.9 KB) - added by fusillicode 4 years ago.
34596.2.diff (5.7 KB) - added by fusillicode 4 years ago.
34596.3.diff (5.4 KB) - added by fusillicode 4 years ago.
34596.patch (6.0 KB) - added by jubstuff 4 years ago.

Download all attachments as: .zip

Change History (12)

@fusillicode
4 years ago

@fusillicode
4 years ago

#1 @fusillicode
4 years ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

I have added the needed return statements to satisfy the enhancement request together with the requested unit tests.

#2 @westonruter
4 years ago

@fusillicode good work! Two pieces of feedback:

1) Instead of returning the value of an assignment:

<?php
return $this->settings[ $setting->id ] = $setting; 

Let's break these up onto separate lines:

<?php
$this->settings[ $setting->id ] = $setting; 
return $setting;

2) I think these are harder to test, but I don't think that these tests are necessarily testing the right thing:

  • test_add_control_returns_a_new_control_instance_created_with_the_arguments_passed_in()
  • test_add_section_returns_a_new_section_instance_created_with_the_arguments_passed_in()
  • test_add_panel_returns_a_new_panel_instance_created_with_the_arguments_passed_in()

So instead of:

<?php
$panel = new WP_Customize_Panel( $this->manager, 'foo' ); 
$inside_panel = $this->manager->add_panel( 'foo' ); 
$this->assertEquals( $panel->id, $inside_panel->id ); 

I think assertions would make more sense like:

<?php
$panel = $this->manager->add_panel( 'foo' ) 
$this->assertInstanceOf( 'WP_Customize_Panel', $panel );
$this->assertEquals( 'foo', $panel->id ); 

@fusillicode
4 years ago

#3 @fusillicode
4 years ago

Thank you a lot for the quick and helpful feedback.

Actually I was expecting both the highlighted points.

I have corrected them by following your advices. I hope that the modification are in line with them. Obviously, If it is not the case, I'll be ready to fix what it is needed to be fixed.

@jubstuff
4 years ago

#4 @jubstuff
4 years ago

Sorry,

I had this in my cache and didn't check out that it was already worked on.

Sorry @fusillicode ;)

#5 @fusillicode
4 years ago

No problem ;)

#6 @westonruter
4 years ago

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

Thanks guys. Let's revisit to commit this once trunk is open for 4.5 enhancements come December.

#7 @westonruter
4 years ago

  • Milestone changed from Future Release to 4.5

#8 @westonruter
4 years ago

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

In 35781:

Customizer: Return added instances for panels, sections, controls, and settings when calling WP_Customize_Manager::add_*() methods.

Add missing phpDoc.

Props fusillicode, jubstuff.
Fixes #34596.

Note: See TracTickets for help on using tickets.