WordPress.org

Make WordPress Core

Opened 19 months ago

Last modified 7 weeks ago

#22080 new enhancement

add_theme_support should merge post-thumbnail post types by default (currently stomps)

Reported by: alexkingorg Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.9
Component: Post Thumbnails Keywords: has-patch 3.6-early
Focuses: Cc:

Description

As documented in the Codex:

http://codex.wordpress.org/Function_Reference/add_theme_support#Post_Thumbnails

you can pass a second param to add_theme_support( 'post-thumbnails' ) to specify the post types you want support enabled for. I expected any post types I passed in to be added to the currently specified post types, instead the passed value stomps the existing value. This means that if you create plugin that registers a post type and enables post-thumbnails for it, you need to write code like this:

https://gist.github.com/3822786

instead of a simple:

add_theme_support( 'post-thumbnails', array( 'my-new-post-type' ) );

The latter makes more sense to me, and I think it's likely other devs might make the same mistake and accidentally stomp on the previous value for post-thumbnails.

The attached patch enables a merge or stomp behavior for post-thumbnails. It defaults to "merge". Passing a bool true as a 3rd param will cause the second param to replace the existing value (current behavior).

Attachments (3)

add-theme-support-without-stomp.diff (1.3 KB) - added by alexkingorg 19 months ago.
add-remove-theme-support-20121016.diff (2.7 KB) - added by alexkingorg 18 months ago.
add-remove-theme-support-20121017.diff (3.1 KB) - added by alexkingorg 18 months ago.

Download all attachments as: .zip

Change History (26)

comment:1 follow-up: scribu19 months ago

  • Keywords 2nd-opinion added

-1 on a boolean third parameter.

But more importantly, why are you calling add_theme_support() from a plugin? It makes zero sense to me.

comment:2 in reply to: ↑ 1 toscho19 months ago

  • Cc info@… added

Replying to scribu:

But more importantly, why are you calling add_theme_support() from a plugin? It makes zero sense to me.

Because if you add thumbnail to the supports argument in register_post_type() the theme support will not be activated automatically, and you get not thumbnail meta box in the edit screen. But you may need exactly that in custom templates.

comment:3 alexkingorg19 months ago

WIthout the boolean 3rd parameter, how do you propose determining if the passed value should merge or stomp?

In the next version of Twitter Tools:

https://github.com/crowdfavorite/wp-twitter-tools

tweets are downloaded and stored as a custom post type. I am enabling Featured Images for just that post type.

comment:4 markoheijnen19 months ago

Third parameter doesn't make sense to me too. You then should be able to do remove_theme_support( 'post-thumbnails', array( 'my-new-post-type' ) );

I can see the usage. I ones needed thumbnail support to a post type because it is used for a mobile app. That isn't really a theme thing.

comment:5 alexkingorg19 months ago

add_theme_support is an API, the usage example in the codex is:

add_theme_support( 'post-thumbnails' );
add_theme_support( 'post-thumbnails', array( 'post' ) );          // Posts only
add_theme_support( 'post-thumbnails', array( 'page' ) );          // Pages only
add_theme_support( 'post-thumbnails', array( 'post', 'movie' ) ); // Posts and Movies

I mistakenly understood this to mean I could add support for other post types by passing them in. I believe others will make the same mistake. The code in the Gist that "fixes" this is simple but was decidedly non-trivial to discover. Requiring that type of interaction with an API makes it likely for people to make mistakes. I think making it easier for them is better.

The action this API currently takes for post-thumbnails would be better named as set_theme_support() rather than add_theme_support().

I also agree that remove_theme_support should accept a post type param for post-thumbnails, but didn't have time to add that to the patch yet.

comment:6 scribu19 months ago

Actually, why do we need to specify if a custom post type supports thumbnails in two places?

To enable thumbnails for pages, this should be enough:

add_post_type_support( 'page', 'thumbnails' );
add_theme_support( 'post-thumbnails' );

And for CPTs you define yourself, you could just set the 'supports' array directly.

Last edited 19 months ago by scribu (previous) (diff)

comment:7 lightningspirit19 months ago

  • Cc lightningspirit@… added

If a theme doesn't support thumbnails by default, no plugin should deauthorize that rule. UI is responsability of the theme and plugins should only extend functionality.
In the case a plugin adds functionality that needs thumbnails and a theme doesn't provide thumbnails, should simply throw an message or notice.
This is my opinion.

comment:8 nacin19 months ago

I doubt that a "no stomp" argument is necessary.

The function is "add", not "set". If you want to remove, you should call remove. We've been careful to merge arguments for other theme support arguments (namely, custom headers and backgrounds), so this patch would make things consistent.

It would make sense that when doing this, remove_theme_support() is updated to handle removing individual post types.

scribu: You need to support it in two places because just because the post type supports thumbnails for a type, doesn't mean the theme does.

comment:9 follow-up: alexkingorg19 months ago

Ok, I'm reading this as: please revise the patch as follows:

  • no "stomp/replace" param (actually makes things simpler)
  • add matching functionality to be able to remove post-thumbnails on a per post-type basis

I generally agree and will proceed with those changes.

The only edge case I can think of is someone trying to explicitly remove support for post-thumbnails from a specific post type when a blanket "support it for all post types" has been set. I'm leaning towards that "doing nothing" and returning an error/false-ish response. Anyone else have a good idea for this situation?

comment:10 in reply to: ↑ 9 scribu19 months ago

Replying to alexkingorg:

The only edge case I can think of is someone trying to explicitly remove support for post-thumbnails from a specific post type when a blanket "support it for all post types" has been set.

They can just call remove_post_type_support( 'foo', 'thumbnails' ), no?

Last edited 19 months ago by scribu (previous) (diff)

comment:11 alexkingorg19 months ago

Good idea, I think that's a solid workaround.

comment:12 alexkingorg18 months ago

Ok, here is the patch with matching support for remove_theme_support( 'post-thumbnails' );

I think a couple of unit tests for this would make sense, so I'll be working on those as well.

comment:13 alexkingorg18 months ago

Unit tests were a good idea, there are bugs in that last patch. Will update shortly. Apologies for the noise here.

comment:14 alexkingorg18 months ago

Ok, the new patch is now working for me with the unit tests attached here:

http://unit-tests.trac.wordpress.org/ticket/136

comment:15 nacin17 months ago

  • Keywords 3.6-early added; 2nd-opinion removed
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement
  • Version changed from trunk to 2.9

comment:16 husobj16 months ago

  • Cc ben@… added

comment:17 flyingtrolleycars13 months ago

  • Cc andrew@… added

comment:18 alexkingorg12 months ago

I don't suppose this is going to make it into 3.6 at this point?

comment:19 follow-up: nacin11 months ago

Related, #21912. Yeah, I kind of hate the current behavior. Find me on the first day of the 3.7 cycle and I will commit it. I think beta is too late for a change in behavior. It would be interesting to brainstorm common use cases where this might break, so we can pre-empt those with blog posts.

comment:20 nacin11 months ago

Maybe the filter proposed in #21912 is a good idea after all.

comment:21 in reply to: ↑ 19 husobj9 months ago

Replying to nacin:

Find me on the first day of the 3.7 cycle and I will commit it.

Is it the first day of 3.7 now?
;)

comment:22 husobj9 months ago

Just to add in another couple of reasons why I think this fix is needed:

  1. If you build a child theme, you may add additional post-thumbnails support for post types that are not supported by the parent theme, therefore you would expect add_theme_support() to merge rather than replace.
  1. Many plugins (ecommerce is a particularly good example) now add their own 'theming' structure so they can work out-of-the-box with no template changes. It would be ideal if they could benefit from using the core Post Thumbnail admin UI without having to resort to a custom solution.

comment:23 SergeyBiryukov7 weeks ago

#27254 was marked as a duplicate.

Note: See TracTickets for help on using tickets.