WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 10 months ago

#15000 assigned enhancement

add_meta_box() should accept array of post types

Reported by: coffee2code Owned by: madalin.ungureanu
Milestone: Awaiting Review Priority: normal
Severity: minor Version: 3.0.1
Component: Administration Keywords: good-first-bug has-patch
Focuses: Cc:

Description

I often find myself defining an identical meta box for different post types. It would be nice if add_meta_box() accepted an array as its $page argument. For backward-compatibility the function will still accept a string. So for all else being equal, the same metabox could be added to multiple post types in a single line of code. Of course, if the meta box needs per-post_type configuration, add_meta_box() could be called separately for each post type.

Example:

add_meta_box( 'my-geo', 'Geolocation Info', array( &$this, 'geo_form' ), 'post', 'side' );
add_meta_box( 'my-geo', 'Geolocation Info', array( &$this, 'geo_form' ), 'page', 'side' );
add_meta_box( 'my-geo', 'Geolocation Info', array( &$this, 'geo_form' ), 'event', 'side' );

would become:

add_meta_box( 'my-geo', 'Geolocation Info', array( &$this, 'geo_form' ), array( 'post', 'page', 'event' ), 'side' );

Of course, the former would continue to work.

One could certainly loop through the post types manually to call add_meta_box(), but I think the proposed patch clarifies coding intention.

Patch is attached. (In the patch, I didn't indent the content of the newly introduced foreach() ... endforeach; since it would look like the whole function got changed when in fact I only made a very minor change. The function also has a precendent of a non-indented foreach. However, I would anticipate the actual commit might want the indentation, which can be provided.)

(Note: affects, or affected by, #13937)

Attachments (9)

15000.diff (1.6 KB) - added by coffee2code 5 years ago.
Aforementioned patch.
15000.2.diff (4.1 KB) - added by iamfriendly 14 months ago.
Refreshed patch. Now accepts an array of post types or an array of screen objects
15000.3.diff (6.0 KB) - added by egill 14 months ago.
15000.4.diff (3.4 KB) - added by egill 14 months ago.
15000.5.diff (4.1 KB) - added by iamfriendly 14 months ago.
Updated patch with changes to remove_meta_box() as well as add_meta_box()
15000.6.diff (2.6 KB) - added by madalin.ungureanu 11 months ago.
Added the same functionality for remove_meta_box() function and made some modiffications in the description of the functions
15000-tests.diff (1.3 KB) - added by mordauk 11 months ago.
15000-tests.diff.2 (3.3 KB) - added by igmoweb 11 months ago.
Unit tests included
1500-tests.3.diff (3.0 KB) - added by igmoweb 11 months ago.
Unit tets diff extension file fixed

Download all attachments as: .zip

Change History (29)

@coffee2code5 years ago

Aforementioned patch.

comment:1 @nacin5 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #13305 which was wontfix'd.

comment:2 @coffee2code5 years ago

A roundabout solution presented there, but so it goes. A straightforward foreach would be more concise:

foreach ( array( 'post', 'page', 'event' ) as $page )
    add_meta_box( 'my-geo', 'Geolocation Info', array( &$this, 'geo_form' ), $page, 'side' );

comment:3 @hakre5 years ago

+1 for wontfix, iterating should be something a developer handles on it's own normally.

comment:4 @ircbot15 months ago

This ticket was mentioned in IRC in #wordpress-dev by tw2113. View the logs.

comment:5 @ericlewis15 months ago

  • Keywords has-patch removed
  • Resolution duplicate deleted
  • Status changed from closed to reopened

I think we should do this. Passing arrays of post types is common practice in other APIs.

Think

WP_Query( array( 'post_type' => array( 'post', 'page' ) ) );

which also supports

WP_Query( array( 'post_type' => 'post' ) );

comment:6 @ericlewis15 months ago

  • Keywords good-first-bug added

comment:7 @ericlewis15 months ago

  • Keywords needs-patch added

Patch needs refresh

comment:8 @SergeyBiryukov15 months ago

  • Milestone set to Awaiting Review

A workaround from comment:7:ticket:13305 that does not require a foreach loop:

function your_callback( $post_type ) {
	if ( in_array( $post_type,  array( 'post', 'page' ) ) ) {
		add_meta_box( 'your-id', 'Title', 'your_box_callback', $post_type );
	}
}
add_action( 'add_meta_boxes', 'your_callback' );
Last edited 15 months ago by SergeyBiryukov (previous) (diff)

@iamfriendly14 months ago

Refreshed patch. Now accepts an array of post types or an array of screen objects

comment:9 @iamfriendly14 months ago

  • Keywords has-patch added; needs-patch removed

Refreshed patch. Now accepts an array of post types or an array of screen objects. Also updated the docblock to reflect that it can accept an array.

@egill14 months ago

@egill14 months ago

comment:10 @egill14 months ago

Personally not a big fan of circular references, would prefer something like:
https://core.trac.wordpress.org/attachment/ticket/15000/15000.3.diff
or even a simple wrapper function for the original function:
https://core.trac.wordpress.org/attachment/ticket/15000/15000.4.diff

Just my $.02 :)

p.s. just a suggestion, haven't tested the patches thoroughly.

Last edited 14 months ago by egill (previous) (diff)

comment:11 @nacin14 months ago

I don't mind the circular reference. However, we also need to think about remove_meta_box(), too.

If you are allowed to pass something to add_meta_box() that actually splits into multiple meta boxes due to individual contexts, then remove is going to end up needing to be called for each individual context. Or, it would need to do the same circular reference.

I just think this complicates things. As to whether that outweighs the syntactical sugar, I'm not sure.

@iamfriendly14 months ago

Updated patch with changes to remove_meta_box() as well as add_meta_box()

comment:12 @iamfriendly14 months ago

I understand what you mean in terms of pros vs cons of this approach, @nacin.

Personally, I don't mind how this is implemented (with circular references or otherwise), but I think the ease of adding/removing metaboxes from multiple screens at once is useful and, it seems to fall in line with what we do elsewhere.

I've updated the patch with additions to remove_meta_box() (as well as slightly tweaking the docs)

Last edited 14 months ago by iamfriendly (previous) (diff)

comment:13 @DrewAPicture12 months ago

  • Owner set to iamfriendly
  • Status changed from reopened to assigned

comment:14 @nacin12 months ago

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

No need to change the spaces in the @param lines — those are accurate as is.

Rather than the ! isset return at the end, I'd just return after the foreach.

We'd also have to make a similar change for remove_meta_box() too. Ideally, we'll add some basic unit tests as well, to tests/phpunit/tests/admin/includesTemplate.php.

@madalin.ungureanu11 months ago

Added the same functionality for remove_meta_box() function and made some modiffications in the description of the functions

comment:15 @madalin.ungureanu11 months ago

  • Keywords has-patch added; needs-patch removed

comment:16 @DrewAPicture11 months ago

  • Owner changed from iamfriendly to madalin.ungureanu

@mordauk11 months ago

comment:17 @mordauk11 months ago

15000-tests.diff includes some basic tests for add/remove_meta_box()

@igmoweb11 months ago

Unit tests included

comment:18 @igmoweb11 months ago

I've included a couple of tests more for this ticket based on @mordauk ones.

I improved the test_remove_meta_box. When we use remove_meta_box, $wp_meta_boxes['post']['advance']['default']['metabox1'] was set to false but the key 'metabox1' was still present.

I splitted the tests so now we have tests for add_meta_box when $screen cannot be an array and when $screen can be an array of post types.

Sorry for the two EOLs in includesScreen.php as I started to make the tests there by mistake.

@igmoweb11 months ago

Unit tets diff extension file fixed

comment:19 @igmoweb11 months ago

Forget about 15000-tests.diff.2​, the extension was wrong, my fault. I included 1500-tests.3.diff

comment:20 @jared_smith10 months ago

  • Keywords needs-unit-tests removed

This ticket has unit tests, so I'm removing the "needs-unit-tests" keyword.

Note: See TracTickets for help on using tickets.