Make WordPress Core

Opened 8 years ago

Last modified 6 years ago

#36666 new enhancement

Enhance `remove_theme_support()` so that it can take additional arguments

Reported by: flixos90's profile flixos90 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.0
Component: Themes Keywords: has-patch dev-feedback has-unit-tests
Focuses: Cc:

Description

Sometimes we might need to use the function remove_theme_support() to only support specific details of a specific theme feature, in particular it would be useful to be able to remove support for specific post formats or remove post thumbnail support for specific post types (related to #22080 for example).

These checks would need to happen in a switch statement, similar to the other theme support functions, and we would need to handle additional arguments.

For example, it should be possible to do remove_theme_support( 'post-formats', 'video' ) to remove support for the video post format.

Attachments (5)

36666.diff (3.1 KB) - added by jmichaelward 8 years ago.
36666.2.diff (3.5 KB) - added by jmichaelward 8 years ago.
36666.3.diff (2.1 KB) - added by flixos90 8 years ago.
updated patch
36666.4.diff (3.4 KB) - added by jmichaelward 8 years ago.
36666.5.diff (3.6 KB) - added by flixos90 8 years ago.
added unit tests

Download all attachments as: .zip

Change History (25)

#1 follow-up: @jmichaelward
8 years ago

@flixos90 I can work on a patch for this. Looking at the remove_theme_support() method, it appears that the function is actually a wrapper method for _remove_theme_support(), which does include a switch statement to conditionally check for some theme settings. I assume the fix would need to be applied to this method instead of its wrapper - can you confirm?

#2 in reply to: ↑ 1 @flixos90
8 years ago

Replying to jmichaelward:

@flixos90 I can work on a patch for this. Looking at the remove_theme_support() method, it appears that the function is actually a wrapper method for _remove_theme_support(), which does include a switch statement to conditionally check for some theme settings. I assume the fix would need to be applied to this method instead of its wrapper - can you confirm?

Sounds great. Yes, the detailed fix should go into _remove_theme_support(), the wrapper function remove_theme_support() only needs to be modified to pass the (optional) additional parameters on to the private function.

@jmichaelward
8 years ago

#3 follow-up: @jmichaelward
8 years ago

@flixos90 I attached a first pass at this, which will need to be fleshed out a bit more for various types of theme support.

Notes:

  1. Optional array values were added to remove_theme_support() and _remove_theme_support().
  1. I moved up the early bail in _remove_theme_support() if the passed feature was not yet added to the theme.
  1. I added a separate switch statement if arguments were passed to the function, so those can be evaluated first. HTML5, Post Format, and Post Thumbnail support are all structured the same, so they share the same set of code for removing arguments.
  1. Upon reviewing the Codex, I noted some features that do not have optional array parameters. I stubbed out those that do (custom-logo and widgets), but left them stubbed for now, as it's not clear to me whether a single option can be simply removed, or if it needs to be reassigned a default depending on its type. I'm assuming the latter.
  1. For non-argument removal, the custom-header-uploads case had a condition. This was previously placed higher in the function and in a separate switch statement. I don't think this was necessary, so I grouped it with the second switch statement. Obviously, we'll want to test to confirm that this is the case.

Let me know how this first pass looks, whether any adjustments are needed to the work that is done here, and how best to approach the remaining theme features, and assuming everything looks good, I'll move forward from this patch.

Thanks.

#4 in reply to: ↑ 3 ; follow-up: @flixos90
8 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

Replying to jmichaelward:
Thanks for this initial patch. I added some comments below (some of which we should probably discuss before improving the patch).

Optional array values were added to remove_theme_support() and _remove_theme_support().

We might consider to also support $args to be a string (as in remove_theme_support( 'post-thumbnails', 'post' )). Not sure about that though since the other theme_support functions don't support it either.

I added a separate switch statement if arguments were passed to the function, so those can be evaluated first. HTML5, Post Format, and Post Thumbnail support are all structured the same, so they share the same set of code for removing arguments.

Do we really need the additional switch statement? I feel like one should be sufficient. We can then check, for each theme feature where we need this, whether $args is used or not. About the grouping, post formats and HTML5 can be together, but post thumbnails work a little differently (keep in mind that this feature might just have the value true in which case we can't remove anything from it).

Upon reviewing the Codex, I noted some features that do not have optional array parameters. I stubbed out those that do (custom-logo and widgets), but left them stubbed for now, as it's not clear to me whether a single option can be simply removed, or if it needs to be reassigned a default depending on its type. I'm assuming the latter.

I think we don't need to consider features like these in here at all. Basically if you provide $args for one these features, just ignore them as it's not meant to be used.

Another thing that we need to think about is whether we wanna explicitly include the $args parameter in the function signature or use func_get_args() like in the other theme support functions. I'm not sure why the other functions handle it like that in the first place, so it would be good to get some feedback by someone who has been involved with these for a longer time.

#5 in reply to: ↑ 4 ; follow-up: @jmichaelward
8 years ago

Thanks for the feedback. I have a few additional comments below:

Replying to flixos90:

We might consider to also support $args to be a string (as in remove_theme_support( 'post-thumbnails', 'post' )). Not sure about that though since the other theme_support functions don't support it either.

My thought here is that remove_theme_support() should mimic add_theme_support() in functionality as closely as possible. Like you indicated, none of the other 'theme_support functions support the passing of a string. I think introducing the possibility for a different argument type here could cause potential developer confusion. Think, for instance, of a dev who wants to add post thumbnail support to one kind of post type, but remove it from another that was registered. If, for some reason, they're aware that they can pass a string to remove_theme_support but not to add_theme_support` and choose to go that route, they'll suddenly have cause to look into the documentation. Requiring the same argument types should streamline this potential issue.

Do we really need the additional switch statement? I feel like one should be sufficient. We can then check, for each theme feature where we need this, whether $args is used or not.

My rationale for splitting them into two statements is that the first switch block is wrapped in a check for arguments, so all cases therein operate on the rationale that arguments are present, and we provide an early return from the function at that point. If no args are present, the next switch block only addresses cases where additional adjustments are needed based on the type of theme support that is being removed (these were all the previously-existing cases). If none of those features match, the function simply removes theme support for the passed feature.

Though combining everything into a single switch statement might seem more elegant, I think the single check is more streamlined and keeps the logic within individual cases shorter and to the point.

About the grouping, post formats and HTML5 can be together, but post thumbnails work a little differently (keep in mind that this feature might just have the value true in which case we can't remove anything from it).

I looked through the documentation before putting this together, and as far as I could tell, the data structures for how HTML5, Post Formats, and Post Thumbnails are stored in the global $_wp_theme_features variable were the same. This would suggest to me that they could be removed in the same fashion. Am I mistaken? Here is a var_dump of some example values:

array (size=3)
  'post-thumbnails' => 
    array (size=1)
      0 => 
        array (size=2)
          0 => string 'page' (length=4)
          1 => string 'post' (length=4)
  'post-formats' => 
    array (size=1)
      0 => 
        array (size=3)
          0 => string 'aside' (length=5)
          1 => string 'gallery' (length=7)
          2 => string 'quote' (length=5)
  'html5' => 
    array (size=1)
      0 => 
        array (size=5)
          0 => string 'comment-list' (length=12)
          1 => string 'comment-form' (length=12)
          2 => string 'search-form' (length=11)
          3 => string 'gallery' (length=7)
          4 => string 'caption' (length=7)

Upon reviewing the Codex, I noted some features that do not have optional array parameters. I stubbed out those that do (custom-logo and widgets), but left them stubbed for now, as it's not clear to me whether a single option can be simply removed, or if it needs to be reassigned a default depending on its type. I'm assuming the latter.

I think we don't need to consider features like these in here at all. Basically if you provide $args for one these features, just ignore them as it's not meant to be used.

That's what I had assumed. Thanks for clearing that up.

Another thing that we need to think about is whether we wanna explicitly include the $args parameter in the function signature or use func_get_args() like in the other theme support functions. I'm not sure why the other functions handle it like that in the first place, so it would be good to get some feedback by someone who has been involved with these for a longer time.

I was curious about that, too. add_theme_support was first introduced in WordPress 3.0, shortly after the release of PHP 5.3. My feeling is that calling this function is more of a legacy approach, and that the larger PHP community as the whole is moving toward more explicit declaration of the parameter types that a function can receive. Adding the parameter to the signature clearly indicates in an editor that the method requires one value and optionally accepts another of a particular type, whereas leaving it out doesn't give this warning.

I'd appreciate others providing their input about this, because if it's preferable to use func_get_args(), then it's certainly trivial to include it.

Thanks again for the feedback and helping guide my approach to this ticket.

#6 in reply to: ↑ 5 @flixos90
8 years ago

Replying to jmichaelward:

About the grouping, post formats and HTML5 can be together, but post thumbnails work a little differently (keep in mind that this feature might just have the value true in which case we can't remove anything from it).

I looked through the documentation before putting this together, and as far as I could tell, the data structures for how HTML5, Post Formats, and Post Thumbnails are stored in the global $_wp_theme_features variable were the same. This would suggest to me that they could be removed in the same fashion. Am I mistaken?

I meant that post thumbnail support and post formats support are a little different from HTML5 support since you can do add_theme_support( 'post-thumbnails' ) which will produce $_wp_theme_features = true - so there's a chance that this variable is not an array (the same applies to post formats). However in this case, we need to think about what the result of something like remove_theme_support( 'post-thumbnails', array( 'post' ) would be: my initial thought says we should return false in this case as it's simply not possible to remove a specific post type from support then.
Basically HTML5 must have additional arguments when adding support while post thumbnails and post formats don't.

About the other issues, let's wait for some more feedback first.

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

#7 @jmichaelward
8 years ago

Ah, gotcha. We could in theory check for all registered post types, but I'm not sure if they're all available at that point. I'll put some thought around this tonight and push up a new version for consideration.

Thanks.

@jmichaelward
8 years ago

#8 follow-up: @jmichaelward
8 years ago

@flixos90, I took your suggestion and broke out post-thumbnails into its own case in my switch block in the latest patch, 36666.2.diff. I'm not crazy about the repeating logic blocks that are shared with the html5/post-formats case, so maybe there's a smarter way to organize this.

That said, I realized that there was a new method introduced in WordPress 4.5 called get_post_types_by_support(), which I could take advantage of in this case. If $_wp_theme_features['post-thumbnails'] is set to a boolean value, we simply reset it to an array and populate that array with the current thumbnail post type support. Then, we process the value just as we did with html5/post-formats.

Ideally, we could perform this check before the switch block, but because there is a naming discrepancy in the reference to post thumbnails (e.g., post-thumbnails and thumbnails), I'm not sure whether we can reconcile at this point.

That said, the method now appears to correctly remove a post type's theme support for post thumbnails if they were previously added without an array value, and I think this is at a point where it could be tested.

Per your previous suggestions, I also removed the case block for the other theme_support features. Let me know what you think.

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

Replying to jmichaelward:

That said, I realized that there was a new method introduced in WordPress 4.5 called get_post_types_by_support(), which I could take advantage of in this case. If $_wp_theme_features['post-thumbnails'] is set to a boolean value, we simply reset it to an array and populate that array with the current thumbnail post type support. Then, we process the value just as we did with html5/post-formats.

Ideally, we could perform this check before the switch block, but because there is a naming discrepancy in the reference to post thumbnails (e.g., post-thumbnails and thumbnails), I'm not sure whether we can reconcile at this point.

That said, the method now appears to correctly remove a post type's theme support for post thumbnails if they were previously added without an array value, and I think this is at a point where it could be tested.

Unfortunately we cannot use the get_post_types_by_support() function since it is targeted add the individual post type support for features like title, editor, comments, thumbnails and so on - it doesn't have to do with theme support. And at the point where most people would call remove_theme_support(), the post types will not have been registered yet, so we need to figure out another way to handle this. I think it's simply not possible to remove a specific post type from support if its current value is just a true.

The same applies to post-formats - this value can be true as well, so we need to ensure we handle this.

@flixos90
8 years ago

updated patch

#10 follow-up: @flixos90
8 years ago

36666.3.diff is an updated patch which builds on the existing switch block. Note that there is no break in the clause for post-thumbnails and post-formats on purpose because the logic in the html5 clause applies to them as well if they haven't returned until then.

The patch will return false when trying to remove a specific element from theme support when the value for this feature is a bool (true).

Also, when removing a specific element from support produces an empty array as result, the theme feature is removed entirely. I assume we don't want to have something like $_wp_theme_features['post-formats'][0] = array().

@jmichaelward Feel free to review and enhance the updated patch. Also, can you ensure that you upload a full diff for upcoming patches please, not only a diff from the file you patched? It is much easier to handle.

#11 in reply to: ↑ 9 @jmichaelward
8 years ago

Replying to flixos90:

Unfortunately we cannot use the get_post_types_by_support() function since it is targeted add the individual post type support for features like title, editor, comments, thumbnails and so on - it doesn't have to do with theme support. And at the point where most people would call remove_theme_support(), the post types will not have been registered yet, so we need to figure out another way to handle this. I think it's simply not possible to remove a specific post type from support if its current value is just a true.

Perhaps I need to clarify: in patch 36666.2.diff, get_post_types_by_support() is only being called if the value for the $_wp_theme_features['post-thumbnails'] is boolean (and, given earlier checks, specifically, that boolean is true). If a post type is registered with theme support for post thumbnails, and add_theme_support( 'post-thumbnails' ) was called somewhere in a plugin or theme, then all post types that support post thumbnails will have them. I think this is a correct use case for this function.

I agree that this cannot be used to incrementally de-register the other types of theme_support, but it should be appropriate for post thumbnails, since it simply indicates which post types support them. In theory, it could be used for post-formats, too, but to my knowledge, WordPress doesn't yet have a built-in mechanism for returning a list of registered post formats (because, as you said, get_theme_support( 'post-formats' ) also returns true. Perhaps this is cause for a new ticket, as well?

Consider this code block:

<?php
<?php
add_action( 'after_setup_theme', function() {
	// outputs true
	var_dump( get_theme_support( 'post-thumbnails' ) );
	
	/* Outputs an array of post types that support post thumbnails, which should be all of them.
	 * In this case, WooCommerce is installed and has registered post thumbnails for all types. It also contains
	 * a custom post type named `product`, which is included in this array.
	 */
	var_dump( get_post_types_by_support( 'thumbnail' ) );
	
	// Let's remove post-thumbnails support for the product post type
	remove_theme_support( 'post-thumbnails', array( 'product' ) );
	
	// Outputs an updated array that excludes the product post type
	var_dump( get_theme_support( 'post-thumbnails' ) );
});

WooCommerce is installed and activated, and it registers a post type called product and calls add_theme_support( 'post-thumbnails' ) if the current theme does not support it. With patch 36666.2.diff applied, the previous block outputs the following:

boolean true
array (size=5)
  0 => string 'post' (length=4)
  1 => string 'page' (length=4)
  2 => string 'attachment:audio' (length=16)
  3 => string 'attachment:video' (length=16)
  4 => string 'product' (length=7)
array (size=1)
  0 => 
    array (size=4)
      0 => string 'post' (length=4)
      1 => string 'page' (length=4)
      2 => string 'attachment:audio' (length=16)
      3 => string 'attachment:video' (length=16)

In terms of the timing, I agree that there are issues. If remove_theme_support() is called in functions.php without a hook in the above case, it is impossible to remove product from the collection because it gets added during after_setup_theme. If, however, the function is called to remove the added post type during the same hook in which is was added, it can be removed. Consider the following code block:

<?php
// Outputs null
var_dump( get_theme_support( 'post-thumbnails' ) );

add_theme_support( 'post-thumbnails' );

// Outputs true
var_dump( get_theme_support( 'post-thumbnails' ) );

remove_theme_support( 'post-thumbnails', array( 'product' ) );

// Outputs an array of post types, but doesn't include 'product'
var_dump( get_theme_support( 'post-thumbnails' ) );

/**
 * Run remove_theme_support() after theme is set up
 */
add_action( 'after_setup_theme', function() {
	// Outputs an array of post types, including product
	var_dump( get_theme_support( 'post-thumbnails' ) );

	remove_theme_support( 'post-thumbnails', array( 'product' ) );

	var_dump( get_theme_support( 'post-thumbnails' ) );
});

And its subsequent output:

boolean false
boolean true
array (size=1)
  0 => 
    array (size=4)
      0 => string 'post' (length=4)
      1 => string 'page' (length=4)
      2 => string 'attachment:audio' (length=16)
      3 => string 'attachment:video' (length=16)
array (size=1)
  0 => 
    array (size=5)
      0 => string 'post' (length=4)
      1 => string 'page' (length=4)
      2 => string 'attachment:audio' (length=16)
      3 => string 'attachment:video' (length=16)
      4 => string 'product' (length=7)
array (size=1)
  0 => 
    array (size=4)
      0 => string 'post' (length=4)
      1 => string 'page' (length=4)
      2 => string 'attachment:audio' (length=16)
      3 => string 'attachment:video' (length=16)

Do note that WooCommerce's current registration of post thumbnail support is problematic, but can be simplified from its current registration to work with this update:

Current:

	/**
	 * Ensure post thumbnail support is turned on.
	 */
	private function add_thumbnail_support() {
		if ( ! current_theme_supports( 'post-thumbnails' ) ) {
			add_theme_support( 'post-thumbnails' );
		}
		add_post_type_support( 'product', 'thumbnail' );
	}

Possible update:

	/**
	 * Ensure post thumbnail support is turned on.
	 */
	private function add_thumbnail_support() {
		add_theme_support( 'post-thumbnails' );
		add_post_type_support( 'product', 'thumbnail' );
	}

This would return the output demonstrated in the previous set of blocks.

I think this demonstrates that get_post_types_by_support is beneficial for this particular use case. Furthermore, I think it's advantageous to the user compared with patch 36666.3.diff, because users are not, in any case, able to remove post type support for an individual post type in this scenario with that patch. They call remove_theme_support( 'post-thumbnails', array( 'post' ), and nothing happens because false is returned.

I propose we continue to explore this solution further, and see whether it can be applied to other types of theme support, including 'html5' and 'post-formats'.

#12 in reply to: ↑ 10 ; follow-up: @jmichaelward
8 years ago

Replying to flixos90:

Also, when removing a specific element from support produces an empty array as result, the theme feature is removed entirely. I assume we don't want to have something like $_wp_theme_features['post-formats'][0] = array().

This is definitely something we'll want to check for, and I had overlooked this in my update to #22080 as well.

@jmichaelward Feel free to review and enhance the updated patch. Also, can you ensure that you upload a full diff for upcoming patches please, not only a diff from the file you patched? It is much easier to handle.

Will do. This is only my third ticket and I don't yet have a lot of experience working with Subversion, so it's not always clear to me when to patch a single file (since that's all I've been editing), or when to patch everything. I've been working from this article. Should I just be ignoring step one and patching everything instead?

#13 in reply to: ↑ 12 ; follow-up: @flixos90
8 years ago

Replying to jmichaelward:
The problem with get_post_types_by_support() is that it does not have to do with theme support. When you call this function on the after_setup_theme hook, it will always return an empty array because the post types are not registered at this point (since after_setup_theme executes before init). That's why I meant we cannot use this function. Sorry, maybe my original post was unclear there.

Will do. This is only my third ticket and I don't yet have a lot of experience working with Subversion, so it's not always clear to me when to patch a single file (since that's all I've been editing), or when to patch everything. I've been working from this article. Should I just be ignoring step one and patching everything instead?

That's perfectly fine, new contributors are always welcome! :) In general, you should always provide full diffs, even if you only edit one file (at least I've never heard anything contrary). Otherwise the workflow to apply patches does not work properly since it won't know which file to patch.

#14 in reply to: ↑ 13 @jmichaelward
8 years ago

Replying to flixos90:

Replying to jmichaelward:
The problem with get_post_types_by_support() is that it does not have to do with theme support. When you call this function on the after_setup_theme hook, it will always return an empty array because the post types are not registered at this point (since after_setup_theme executes before init). That's why I meant we cannot use this function. Sorry, maybe my original post was unclear there.

I had to set this ticket side for a few days to think about it, as you're right that custom post types, when registered correctly, are not retrieved by get_post_types_by_support. It does not appear, however, that get_post_types_by_support returns an empty array when called. Instead, it returns four: post, page, attachment:video, and attachment:audio These same four array values are returned whether get_post_types_by_support is called outside of an action or within one, such as init.

Something that threw before is that WooCommerce appears to be registering its product post type sooner than init (which, of course, goes against the documentation), so it was getting returned in my previous attempts to resolve this ticket.

My question now is: what if we're chasing the wrong problem? What if the issue here is the idea that theme support is sometimes stored as a boolean value (when options are not passed), and sometimes stored as an array?

Ticket #22080 perpetuated this thinking a bit, as we simply exited if theme support was already set. What if, instead, we just never saved theme_support as a boolean?

My latest patch (36666.4.diff) updates add_theme_support to set $args to the value of get_post_types_by_support( 'thumbnail') if either $args or $_wp_theme_features[ 'post-thumbnails' ] is set to true. From then on, any post type that registers and adds theme support for post thumbnails will have its post type pushed to the array, as before. In addition, post thumbnail support can be removed in functions.php, as long as the function is called during the same action (or later) in which a post type's support was added. So, in the case of WooCommerce, post thumbnail support for products can be removed in after_setup_theme (but not before), and in the case of other custom post types, remove_theme_support must be called in init for it to work. Otherwise, it just fails silently.

Also, since HTML5, Post Formats, and Post Thumbnails would ultimately run the same kind of check, I decided to take @flixos90's previous advice and put everything back into a single switch, because it's far more streamlined.

Potential issues for consideration:

  1. Currently, when users register a custom post type and indicate that it supports a theme feature, such as post thumbnails, that support is automatically updated. That would not be the case with this patch, so revisions would need to be made to append the post type to the array for post thumbnails (and eventually html5 and post formats).
  1. Theme and plugin developers may be running a check to see if a theme currently supports a particular feature. Hard (===) checks for post thumbnails would fail, because the value would no longer be boolean. However, populated arrays are truthy in PHP, so this change shouldn't have any additional effect.

All this said, I don't consider this patch "done" by any means, but welcome discussion on the potential positives and challenges with implementing this approach.

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

@jmichaelward
8 years ago

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


8 years ago

#16 @swissspidy
8 years ago

From what I've just read now, this ticket would benefit from one main thing: tests.

For example, a quick look at 36666.4.diff reveals that _remove_theme_support( 'custom-header-uploads' ) won't work anymore with that patch because the function bails early for non-existent feature, thus breaking backwards compatibility.

I'd suggest writing plenty of tests for the current behaviour of remove_theme_support(), then adding tests for the suggested $args param and finally the actual implementation.

Regarding the patch, I wouldn't just add braces everywhere because we can, but only on lines we actually touch. Looks like a mess otherwise.

#17 @flixos90
8 years ago

  • Keywords needs-unit-tests added

@flixos90
8 years ago

added unit tests

#18 @flixos90
8 years ago

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

In 36666.5.diff I added unit tests, one function for the custom-header-uploads feature and the other to actually test removing subsets. This patch builds upon 36666.3.diff since 36666.4.diff would fail on the custom-header-uploads test and since get_post_types_by_support() cannot be used by theme support functions at this point.

This ticket was mentioned in Slack in #docs by ambrosey. View the logs.


8 years ago

This ticket was mentioned in Slack in #themereview by joyously. View the logs.


6 years ago

Note: See TracTickets for help on using tickets.