Make WordPress Core

Opened 4 years ago

Closed 2 years ago

Last modified 2 years ago

#51657 closed defect (bug) (fixed)

Correct displaying the _doing_it_wrong() message for add_theme_support( 'html5' )

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-screenshots has-patch needs-dev-note
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Background: #24932, #51390.

As a result of [25193], [25235], and [25785] add_theme_support( 'html5' ) is supposed to return false and display a _doing_it_wrong() notice: "You need to pass an array of types".

As far as I can tell, this has never worked as expected, due to ! is_array( $args[0] ) check being in the elseif condition after empty( $args[0] ), which always succeeds in this case.

Currently, calling add_theme_support( 'html5' ) without passing an array of supported types just silently falls back to array( 'comment-list', 'comment-form', 'search-form' ) for backward compatibility, without any indication that it is not the recommended approach.

Attachments (2)

51657.diff (951 bytes) - added by audrasjb 3 years ago.
Themes: Correct displaying the _doing_it_wrong() message for add_theme_support( 'html5' )
Capture d’écran 2021-01-11 à 00.26.37.png (274.9 KB) - added by audrasjb 3 years ago.
_doing_it_wrong notice

Download all attachments as: .zip

Change History (17)

#1 @SergeyBiryukov
4 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#2 @SergeyBiryukov
4 years ago

  • Description modified (diff)

@audrasjb
3 years ago

Themes: Correct displaying the _doing_it_wrong() message for add_theme_support( 'html5' )

@audrasjb
3 years ago

_doing_it_wrong notice

#3 @audrasjb
3 years ago

  • Keywords has-screenshots has-patch added

Nice catch @SergeyBiryukov!

I think 51657.diff does the job :)

Just wonder if we want to change the version param. I think it's fine to keep the reference to the version it was added, but on another hand, it would be nice to upgrade the version to 5.7.0 as the notice will be displayed for the very first time. Any thought?

#4 @audrasjb
3 years ago

  • Keywords needs-dev-note added

Adding needs-dev-note workflow keyword. It won't need a full dev note, but a quick mention in the Miscellaneous changes won't hurt :)

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


3 years ago

#6 @peterwilsoncc
3 years ago

  • Keywords needs-refresh needs-unit-tests added

Reviewing 51657.diff :

  • The condition can be reduced to if ( empty( $args[0] ) || ! is_array( $args[0] ) ) -- empty() is a short cut for ! isset($var) || ! $var.
  • I haven't run the full test suite but I am getting a few failures in Tests_Theme_Support
  • Some additional tests to make sure the feature is throwing a doing it wrong as expected would be ideal too
> phpunit --filter Tests_Theme_Support

.....FFF....                                                      12 / 12 (100%)

There were 3 failures:

1) Tests_Theme_Support::test_supports_html5
Unexpected incorrect usage notice for add_theme_support( 'html5' )
Failed asserting that an array is empty.

/vagrant/wordpress-develop/tests/phpunit/includes/abstract-testcase.php:516
/vagrant/wordpress-develop/tests/phpunit/includes/abstract-testcase.php:528

2) Tests_Theme_Support::test_supports_html5_subset
Failed asserting that null is false.

/vagrant/wordpress-develop/tests/phpunit/tests/theme/support.php:101

3) Tests_Theme_Support::test_supports_html5_invalid
Failed asserting that null is false.

/vagrant/wordpress-develop/tests/phpunit/tests/theme/support.php:130

#7 @SergeyBiryukov
3 years ago

  • Milestone changed from 5.7 to 5.8

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


3 years ago

#9 @JeffPaul
3 years ago

  • Milestone changed from 5.8 to Future Release

Given that the needs-refresh keyword was added in before this got milestoned AND no further actions since then, I'm going to punt this to Future Release with the 5.8 Beta 1 release tomorrow.

#10 @SergeyBiryukov
2 years ago

  • Milestone changed from Future Release to 6.0

#11 @SergeyBiryukov
2 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 52828:

Themes: Correct the logic for displaying a _doing_it_wrong() notice for add_theme_support( 'html5' ).

  • Calling add_theme_support( 'html5' ) without passing an array of supported types should throw a _doing_it_wrong() notice: "You need to pass an array of types".
  • If the second parameter is not specified, it should fall back to an array of comment-list, comment-form, and search-form for backward compatibility.
  • If the second parameter is not an array, the function should return false.

The latter two points are covered by existing unit tests. The first one is now addressed by @expectedIncorrectUsage.

Follow-up to [25193], [25235], [25785].

Props audrasjb, peterwilsoncc, SergeyBiryukov.
Fixes #51657.

#12 @SergeyBiryukov
2 years ago

  • Keywords needs-refresh needs-unit-tests removed

#13 follow-up: @bobbingwide
2 years ago

Just wonder if we want to change the version param. I think it's fine to keep the reference to the version it was added, but on another hand, it would be nice to upgrade the version to 5.7.0 as the notice will be displayed for the very first time. Any thought?

I've just spent a while trying to understand why I've not seen this message before WordPress 6.0
I agree that the original doing_it_wrong was added in 3.6.1.

But up to 5.9.3 everything was OK since the code magically defaulted the args array.
It would have been nice to have seen something that indicated there'd been a further change in 6.0

#14 @SergeyBiryukov
2 years ago

In 53513:

Docs: Add @since notes for html5 theme feature changes in add_theme_support():

  • As of WordPress 3.6.1, the html5 feature requires an array of types to be passed. Defaults to comment-list, comment-form, search-form for backward compatibility.
  • As of WordPress 6.0, the previously added _doing_it_wrong() notice is actually displayed as expected.

Follow-up to [25193], [25235], [25785], [49344], [49354], [52828].

Props bobbingwide.
See #51657, #55646.

#15 in reply to: ↑ 13 @SergeyBiryukov
2 years ago

Replying to bobbingwide:

It would have been nice to have seen something that indicated there'd been a further change in 6.0

You're absolutely right, thanks for bringing this up! [53513] adds the corresponding @since notes.

Note: See TracTickets for help on using tickets.