Make WordPress Core

Opened 11 years ago

Closed 8 years ago

Last modified 8 years ago

#22080 closed enhancement (fixed)

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

Reported by: alexkingorg's profile alexkingorg Owned by: rachelbaker's profile rachelbaker
Milestone: 4.6 Priority: normal
Severity: normal Version: 2.9
Component: Post Thumbnails Keywords: has-patch has-unit-tests
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 (7)

add-theme-support-without-stomp.diff (1.3 KB) - added by alexkingorg 11 years ago.
add-remove-theme-support-20121016.diff (2.7 KB) - added by alexkingorg 11 years ago.
add-remove-theme-support-20121017.diff (3.1 KB) - added by alexkingorg 11 years ago.
22080-merge-post-thumbnails-cpt.diff (878 bytes) - added by jmichaelward 8 years ago.
22080.2.diff (2.0 KB) - added by flixos90 8 years ago.
minor fix, unit tests
22080.3.diff (875 bytes) - added by jmichaelward 8 years ago.
Update 22080.2.diff - add periods to inline comments, per WP style guide.
22080.4.diff (2.0 KB) - added by jmichaelward 8 years ago.

Download all attachments as: .zip

Change History (45)

#1 follow-up: @scribu
11 years 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.

#2 in reply to: ↑ 1 @toscho
11 years 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.

#3 @alexkingorg
11 years 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.

#4 @markoheijnen
11 years 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.

#5 @alexkingorg
11 years 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.

#6 @scribu
11 years 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 11 years ago by scribu (previous) (diff)

#7 @lightningspirit
11 years 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.

#8 @nacin
11 years 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.

#9 follow-up: @alexkingorg
11 years 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?

#10 in reply to: ↑ 9 @scribu
11 years 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 11 years ago by scribu (previous) (diff)

#11 @alexkingorg
11 years ago

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

#12 @alexkingorg
11 years 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.

#13 @alexkingorg
11 years ago

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

#14 @alexkingorg
11 years ago

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

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

#15 @nacin
11 years 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

#16 @husobj
11 years ago

  • Cc ben@… added

#17 @flyingtrolleycars
11 years ago

  • Cc andrew@… added

#18 @alexkingorg
11 years ago

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

#19 follow-up: @nacin
11 years 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.

#20 @nacin
11 years ago

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

#21 in reply to: ↑ 19 @husobj
11 years 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?
;)

#22 @husobj
11 years 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.

#23 @SergeyBiryukov
10 years ago

#27254 was marked as a duplicate.

#24 @chriscct7
8 years ago

  • Keywords needs-refresh added; 3.6-early removed

#25 @jmichaelward
8 years ago

Hello everyone,

I am proposing revisiting this ticket, because I believe the current implementation of add_theme_support( 'post-thumbnails' ) to be somewhat confusing at best, and at worst, it may introduce conflicts between plugins and themes.

As an example, I was reviewing the current version of WooCommerce (version 2.4.7 at the time of this writing) when I noticed that line 348 of woocommerce.php declares a function titled add_thumbnail_support(). This function runs a check to see whether the currently-installed theme supports post thumbnails. If it doesn't, the plugin adds post thumbnail theme support for all installed post types, then attaches that theme support the WooCommerce product post type, as well.

This could be potentially confusing to an end user who did not have post thumbnail support previously activated in their theme, and upon activating WooCommerce, now sees the option to add featured images to all of their pages, posts, and any other custom post type that is registered in the system. Furthermore, should they choose to add a featured image to those previously-unsupported post types, confusion may be compounded when they do not see those images displayed on the front-end.

Currently, add_theme_support( 'post-thumbnails' ) accepts an additional array value for which post types should include post thumbnail support. Ideally, plugin authors would be able to do something along these lines:

add_theme_support( 'post-thumbnails', array( 'my_custom_post_type' ) );

...which would append their custom post type to an array of other post types that have post thumbnail support enabled. If none are enabled, this would register post thumbnail support and assign it to that post type. If, at that time, the end user installs another plugin or a theme that adds specified post thumbnail support in this manner, there would be no conflicts as to which post types would be displaying the option to attach a featured image.

I just came across this issue today, so I don't have a proposed resolution just yet, but I do think we can improve upon the previous solutions that have been proposed. I'll plan to write something up and submit it in the next few days. In the meantime, I wanted to re-open this issue for discussion.

Last edited 8 years ago by jmichaelward (previous) (diff)

#26 @flixos90
8 years ago

Adding the post types to a possibly existing array in $_wp_theme_features['post-thumbnails'] makes sense in my opinion as well (similar like it happens for the html5 theme support feature). I think implementing this would be sufficient for a first iteration.

To me, the enhancement of remove_theme_support() is a different (more global) topic as we currently do not allow any specific details of a feature to be removed - only the complete feature. So I don't think we should change this here, rather create a different ticket for this if it is desirable.

@jmichaelward:

I just came across this issue today, so I don't have a proposed resolution just yet, but I do think we can improve upon the previously solutions that have been proposed. I'll plan to write something up and submit it in the next few days. In the meantime, I wanted to re-open this issue for discussion.

Still interested in providing an updated patch for add_theme_support()? :)

#27 @jmichaelward
8 years ago

@flixos90 Thanks for pinging me on this - it apparently fell off my radar very quickly after posting. I'll take a look at the current implementation and draft up something in the next day or two.

#28 follow-up: @jmichaelward
8 years ago

I'm digging back into this issue, and I think some conversation is warranted with regard to exactly how this functionality should work before moving forward with any potential code fixes. The original issue and my response six months ago may not be entirely evident, so I'm hoping to provide a bit more clarity here so we can confirm whether this is in fact an issue at all and instead desired functionality by the add_theme_support() mechanism.

The Codex states that, as of version 2.9, developers can indicate that they wish to add post thumbnails support to their theme. As Alex King reported four years ago, they can do so by either writing add_theme_support( 'post-thumbnails' ); in their functions.php file, or by optionally passing an array of post types they wish to support.

According to the Codex, users "can optionally pass a second argument with an array of the Post Types for which you want to enable this feature.".

When adding theme support without this optional array, there are no issues. However, when an array is passed, WordPress is told only to support post thumbnails for the provided post types.

To the original issue submission, @scribu asked, "...more importantly, why are you calling add_theme_support() from a plugin? It makes zero sense to me." Naturally, plugins such as WooCommerce desire to do exactly this, so they can provide post thumbnail supports for their product pages. Unfortunately, they're activating post thumbnails for all post types (without passing a parameter), when I believe their intention is to add theme support only for their product post type, but that's merely a related issue, though it may be a byproduct of this one.

In WooCommerce's case, because they activate post thumbnails for all post types, developers don't need to add support for them to their theme at all. But this is actually the crux of the issue. A less-experienced developer may read the Codex and opt to pass an array of post types that don't include those that WooCommerce (or other third-party plugin developers) registered, and thus, override the post type support for all post types in which it was previously enabled.

At this point, there is a question of intent. A developer who calls, for instance, add_theme_support( 'post-thumbnails, array( 'post' ) ); may in fact be intending to disable post thumbnails for all other post types where it was previously supported, but as @scribu indicated, there is another method to do that: remove_post_type_support().

I agree with @nacin's assessment that intent here is key, particularly when we're working with plugins that register post types outside of the theme (and if we're wanting to continue to promote user's ability to switch between themes without losing their content, this is important). Toward that end, add_theme_support() needs to correctly add support for a post type when an array is passed, and not squash any previously-added types, such as WooCommerce's thumbnail support for product. remove_post_type_support() works as it should, and if a developer adds it to their functions.php file, it's clear that their intent was to remove thumbnail support for that type (again, such as for WooCommerce's product post type).

Thus, I propose updating add_theme_support() to build an array of post types that support post thumbnails, and will take on the work of doing so once I hear feedback from others.

#29 @jmichaelward
8 years ago

The easiest way to replicate this issue is as follows:

  1. Install and activate WooCommerce. It registers a custom post type called product and adds post thumbnail support to all post types (so you can go to a Page, Post, or Product and see that you can attach an image).
  1. Create a new theme with a functions.php file, and enter add_theme_support( 'post-thumbnails', array( 'post' ) );.
  1. Click into a new or existing post, and confirm that you're able to attach a featured image.
  1. Click into a new or existing page or WooCommerce product, and note that you no longer have the ability to attach a featured image to these types.
Last edited 8 years ago by jmichaelward (previous) (diff)

#30 @jmichaelward
8 years ago

I just uploaded a proposed patch to this issue. It checks for two cases when users attempt to add post-thumbnail support to a theme:

  1. If get_theme_support( 'post-thumbnails' ) returns a boolean value of true, the function returns. Post thumbnails have already been declared for all post types.
  1. Next, if the arguments are an array and the global $_wp_theme_features['post-thumbnails'] value is an array, those values will get merged and non-unique values will be discarded. These new arguments get applied at the end of the function.

In the third-party realm, this code change can cause some compatibility issues depending on the checks that are being run. In WooCommerce's case, they're only calling add_theme_support( 'post-thumbnails' ) if the current theme supports post thumbnails. With the fixed case, this would be true when a user adds, for instance, add_theme_support( 'post-thumbnails', array( 'post' ) ); to functions.php, as post thumbnails would no longer be available to WooCommerce's product post type because the check would return true. This can be fixed by Woo by updating their conditional check from:

function add_thumbnail_support() {
    if ( ! current_theme_supports( 'post-thumbnails' ) ) {
        add_theme_support( 'post-thumbnails' );
    }

    add_post_type_support( 'product', 'thumbnail' );
}

to:

function add_thumbnail_support() {
    if ( ! current_theme_supports( 'post-thumbnails' ) || ! in_array( 'product', get_theme_support( 'post-thumbnails' ) ) ) {
	    add_theme_support( 'post-thumbnails', array( 'product' ) );
    }

    add_post_type_support( 'product', 'thumbnail' );
}

The caveat here is there's no telling how many plugins might be running similar checks. I'd appreciate any feedback or insight others can provide into this issue.

@flixos90
8 years ago

minor fix, unit tests

#31 in reply to: ↑ 28 ; follow-up: @flixos90
8 years ago

  • Keywords has-unit-tests added; needs-refresh removed
  • Milestone changed from Future Release to 4.6

The patch looks good. In 22080.2.diff I fixed just a minor issue (function needs to return void instead of true when successful), plus I added some unit tests.

Replying to jmichaelward:

At this point, there is a question of intent. A developer who calls, for instance, add_theme_support( 'post-thumbnails, array( 'post' ) ); may in fact be intending to disable post thumbnails for all other post types where it was previously supported, but as @scribu indicated, there is another method to do that: remove_post_type_support().

I'll open another ticket to enhance remove_theme_support() so that it can be used to remove particular parts of what a theme supports so that this proposal can actually work. Without a change in remove_theme_support(), it wouldn't be possible to remove a specific post type (in case we explicitly don't want it to support post thumbnails).

#33 in reply to: ↑ 31 ; follow-up: @rachelbaker
8 years ago

Replying to flixos90:

The patch looks good. In 22080.2.diff I fixed just a minor issue (function needs to return void instead of true when successful), plus I added some unit tests.

This is looking good. @flixos90 or @jmichaelward could you update the latest patch (22080.2.diff) so the inline comments end in a period? That brings them inline with our PHP Documentation Standards.

@jmichaelward
8 years ago

Update 22080.2.diff - add periods to inline comments, per WP style guide.

#34 in reply to: ↑ 33 @jmichaelward
8 years ago

@rachelbaker Thanks for the catch - I was responsible for the initial comments, so I've updated the comments in @flixos90's patch to adhere to the WP style guide. Let me know if any additional adjustments are needed.

#35 @flixos90
8 years ago

@jmichaelward your latest patch doesn't include the unit tests anymore, it's only the theme.php. Can you create a full diff of the setup, including the unit tests (like in 22080.2.diff)? Then the patch should be good to go :)

Last edited 8 years ago by flixos90 (previous) (diff)

@jmichaelward
8 years ago

#36 @jmichaelward
8 years ago

My apologies. I have attached an updated patch that includes the unit tests.

#37 @rachelbaker
8 years ago

  • Owner set to rachelbaker
  • Resolution set to fixed
  • Status changed from new to closed

In 37308:

Post Thumbnails: When using add_theme_support( ‘post-thumbnails’, array( $post_types) ) merge the supported post_types.

Allow the adding of post-thumbnail support for one or more post_types without unsetting any previously added post_types. This matches the behavior of other uses of add_theme_support() and the expectations of a function with a prefix of “add”.
To unset post-thumbnail support use remove_theme_support() instead.

Fixes #22080

Props alexkingorg, jmichaelward, and flixos90.

#38 @rachelbaker
8 years ago

In 37313:

Post Thumbnails: Fix logic bug and tests from [37308] where post-thumbnails support wasn’t added if there were no previous post_types with support already.

See #22080

Note: See TracTickets for help on using tickets.