[wp-trac] [WordPress Trac] #36666: Enhance `remove_theme_support()` so that it can take additional arguments

[WordPress Trac]

#36666: Enhance `remove_theme_support()` so that it can take additional arguments
------------------------------------+------------------------------
 Reporter:  flixos90                |       Owner:
     Type:  enhancement             |      Status:  new
 Priority:  normal                  |   Milestone:  Awaiting Review
Component:  Themes                  |     Version:  3.0
 Severity:  normal                  |  Resolution:
 Keywords:  has-patch dev-feedback  |     Focuses:
------------------------------------+------------------------------

Comment (by jmichaelward):

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

 Replying to [comment:4 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.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/36666#comment:5>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform