WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#27278 closed defect (bug) (fixed)

Clarifying add_theme_support() inline docs for 'html5'

Reported by: jond3r Owned by: 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 6 years ago.
27278.2.diff (1.8 KB) - added by jond3r 6 years ago.
27278.3.diff (2.4 KB) - added by jond3r 6 years ago.
27278.4.diff (791 bytes) - added by SergeyBiryukov 6 years ago.
27278.5.diff (1.9 KB) - added by jond3r 6 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 6 years ago.
A minimal patch. Only adding comment.
27278.7.diff (974 bytes) - added by jond3r 6 years ago.
A modified minimal patch.
27278.8.diff (974 bytes) - added by jond3r 6 years ago.
Just a refresh

Download all attachments as: .zip

Change History (22)

@jond3r
6 years ago

@jond3r
6 years ago

@jond3r
6 years ago

#1 @jond3r
6 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
6 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 6 years ago by SergeyBiryukov (previous) (diff)

@jond3r
6 years ago

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

#3 @nathanrice
6 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
6 years ago

Yes, door 4 please.

#5 @jond3r
6 years ago

#27292 suggests a new html5 type: 'caption'

#6 @jond3r
6 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.

Version 0, edited 6 years ago by jond3r (next)

@jond3r
6 years ago

A minimal patch. Only adding comment.

#7 follow-up: @SergeyBiryukov
6 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
6 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
6 years ago

A modified minimal patch.

#9 @jond3r
6 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
6 years ago

Just a refresh

#10 @jond3r
6 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
6 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
6 years ago

  • Milestone changed from Awaiting Review to 3.9

#13 @DrewAPicture
6 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
6 years ago

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