Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#27278 closed defect (bug) (fixed)

Clarifying add_theme_support() inline docs for 'html5'

Reported by: jond3r's profile jond3r Owned by: drewapicture's profile DrewAPicture
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.9
Component: Themes Keywords: has-patch
Focuses: docs Cc:

Description

In #26697 was introduced support for HTML5 markup in image galleries. To enable this you would have to write:

add_theme_support( 'html5', array( 'gallery' ) );

Previously there were just three so-called 'types' of HTML5 support in core: 'comment-list', 'comment-form', 'search-form'.
So now there is one more: 'gallery'!

This has pinpointed a weakness (or asymmetry if you like) in the current API for theme support. Though discouraged (ticket:26697#comment:12), it is currently possible and legitimate to use:

add_theme_support( 'html5' );

which adds support for the three types 'comment-list', 'comment-form' and 'search-form', but not the new type 'gallery'!
Thus, as the code stands today 'gallery' is treated as a second-class citizen, which it does not deserve!

Another manifestation of the current asymmetry of the API is that remove_theme_support( 'html5' ) removes all previously added types, but add_theme_support( 'html5' ) only adds back three of them.

To add some sanity back to the theme support API, three possible roads are outlined:

  1. Keep the current code as is, for preserved backwards compatibility, but add some comment to the code and an additional assertion for clarity to the theme-support test. This is provided in the attached patch no. 1.
  1. Add 'gallery' to the default list of types for preserved symmetry but broken back-compat, with some additional test assertion. This is patch no. 2.
  1. Remove the possibility to use add_theme_support( 'html5' ) altogether, which breaks back-compat this time, but is future proof as other types are added. It also restores symmetry of the API, albeit in another form. Adjust the unit tests accordingly. This is patch no. 3.

I do not have very strong feelings about which road is better. No. 1 is safest, but does not provide much improvement. Patch no. 2 was rejected in ticket:26697#comment:12, and will realistically not be accepted as it is not future proof and breaks back-compat in a silent way: gallery styling might stop working for themes or plugins using add_theme_support( 'html5' ). Patch no. 3 is 100% future-proof, but usage of add_theme_support( 'html5' ) will trigger a doing-it-wrong error. It is somewhat 'boring' though, as something is taken away, and a longer api-call must be made to achieve the same thing as before.

Much of the current behavior was added in #24932.

Attachments (8)

27278.1.diff (1.5 KB) - added by jond3r 10 years ago.
27278.2.diff (1.8 KB) - added by jond3r 10 years ago.
27278.3.diff (2.4 KB) - added by jond3r 10 years ago.
27278.4.diff (791 bytes) - added by SergeyBiryukov 10 years ago.
27278.5.diff (1.9 KB) - added by jond3r 10 years ago.
Per Sergey's suggestion, but adding @return-tag to doc block and updated unit tests
27278.6.diff (1.0 KB) - added by jond3r 10 years ago.
A minimal patch. Only adding comment.
27278.7.diff (974 bytes) - added by jond3r 10 years ago.
A modified minimal patch.
27278.8.diff (974 bytes) - added by jond3r 10 years ago.
Just a refresh

Download all attachments as: .zip

Change History (22)

@jond3r
10 years ago

@jond3r
10 years ago

@jond3r
10 years ago

#1 @jond3r
10 years ago

  • Keywords has-patch added
  • Summary changed from More sanity for add_theme_support( 'html5' ) to More sanity to add_theme_support( 'html5' )

#2 @SergeyBiryukov
10 years ago

Though discouraged (ticket:26697#comment:12), it is currently possible and legitimate to use:
add_theme_support( 'html5' );
which adds support for the three types 'comment-list', 'comment-form' and 'search-form', but not the new type 'gallery'! Thus, as the code stands today 'gallery' is treated as a second-class citizen, which it does not deserve!

That list is just for backwards compatibility (see [25235]), new items should not be added there.

27278.4.diff adds a _doing_it_wrong() call and a "Back compat" comment, per comment:12:ticket:26697. I think that's all we need here. We didn't do that originally, as the consensus in comment:12:ticket:24932 was that _doing_it_wrong() was not necessary until we actually had a new markup in some other function. Now that we've added it to gallery_shortcode(), let's do this.

Last edited 10 years ago by SergeyBiryukov (previous) (diff)

@jond3r
10 years ago

Per Sergey's suggestion, but adding @return-tag to doc block and updated unit tests

#3 @nathanrice
10 years ago

Option 1 seems sensible to me. It doesn't relegate gallery to any non-preferred status, it's just a fallback for people who implemented html5 support before gallery support existed. If all the documentation recommends being explicit with what elements you want on, use of html5 without explicit support will go way down.

This preserves backward compat without breaking sites.

Removing the ability to use html5 by itself isn't "future proof as other types are added" any more than option 1 is. All it does is forces unnecessary failures with the only real benefit being the removal of some code.

As far as I'm concerned, this doesn't even warrant a _doing_it_wrong trigger. Themes (for the most part) don't exactly have a way of pushing updates out to users.

#4 @obenland
10 years ago

Yes, door 4 please.

#5 @jond3r
10 years ago

#27292 suggests a new html5 type: 'caption'

#6 @jond3r
10 years ago

The general message here seems to be: "Change as little as possible". Still I think it would be valuable to have some comment in the code to indicate what is considered as deprecated usage (some devs might not read the codex).

The attached .6 patch is essentially a modified version of .1 that tries to better adhere to the documentation standards (replacing 'void' with 'mixed' in the @return line). The unit tests are also not touched.

Edit: I raised the issue of using void in mixed type @return here: comment:19:ticket:27098

Last edited 10 years ago by jond3r (previous) (diff)

@jond3r
10 years ago

A minimal patch. Only adding comment.

#7 follow-up: @SergeyBiryukov
10 years ago

I don't really see the benefit of adding a @return value here.

This function doesn't have a consistent return value (nor do I think it should), so why would anyone check it? The _doing_it_wrong() message is pretty clear.

Let's just add a comment from 27278.6.diff.

#8 in reply to: ↑ 7 @jond3r
10 years ago

Replying to SergeyBiryukov:

I don't really see the benefit of adding a @return value here.

I agree to some extent. I think it is best to have a consistent return type, bool e.g., but that should have been thought of at design time.

@jond3r
10 years ago

A modified minimal patch.

#9 @jond3r
10 years ago

Patch .7 restores the use of @return void|bool as I think this is accepted usage of void, based on for example [27575].

The @return line can be skipped if deemed unnecessary (though I think it should be added).

@jond3r
10 years ago

Just a refresh

#10 @jond3r
10 years ago

  • Summary changed from More sanity to add_theme_support( 'html5' ) to Clarifying comment in add_theme_support() for 'html5'

Changed the title to more accurately describe the current patch.

#11 @DrewAPicture
10 years ago

  • Focuses docs added
  • Summary changed from Clarifying comment in add_theme_support() for 'html5' to Clarifying add_theme_support() inline docs for 'html5'

#12 @johnbillion
10 years ago

  • Milestone changed from Awaiting Review to 3.9

#13 @DrewAPicture
10 years ago

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

In 27879:

Clarify inline documentation for the 'html5' case and PHPDoc return in add_theme_support().

Props jond3r.
Fixes #27278.

#14 @DrewAPicture
10 years ago

  • Type changed from enhancement to defect (bug)
Note: See TracTickets for help on using tickets.