WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 18 months ago

Last modified 18 months ago

#24900 closed defect (bug) (fixed)

PHPDocs for $wp_admin_bar in admin-bar.php

Reported by: jeremyfelt Owned by:
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.1
Component: Inline Docs Keywords: has-patch
Focuses: Cc:

Description

Though the class instantiated for $wp_admin_bar and used throughout wp-includes/admin-bar.php can be modified via a filter, the chance is high enough that it will be WP_Admin_Bar that we should hint at the type with PHPDocs to make intentions clearer.

Attachments (4)

24900.diff (3.8 KB) - added by jeremyfelt 19 months ago.
24900.2.diff (3.3 KB) - added by jeremyfelt 19 months ago.
Previous patch included non relevant changes in other file
24900.3.diff (3.7 KB) - added by DrewAPicture 19 months ago.
formatting
24900.4.diff (3.7 KB) - added by jeremyfelt 19 months ago.
Correct order for @param and @var docs to type, then name

Download all attachments as: .zip

Change History (13)

@jeremyfelt19 months ago

@jeremyfelt19 months ago

Previous patch included non relevant changes in other file

@DrewAPicture19 months ago

formatting

comment:1 @DrewAPicture19 months ago

  • Cc xoodrew@… added
  • Milestone changed from Awaiting Review to 3.7

comment:2 @nacin19 months ago

* @param $wp_admin_bar WP_Admin_Bar. should actually be * @param WP_Admin_Bar $wp_admin_bar — type comes before name. Since there is no explanation, there should also not be a period.

@jeremyfelt19 months ago

Correct order for @param and @var docs to type, then name

comment:3 @jeremyfelt19 months ago

Well then, there's that. :) Refreshed patch 24900.4.diff gets the order right and ditches the period.

Last edited 19 months ago by jeremyfelt (previous) (diff)

comment:4 @ryan19 months ago

In 24999:

phpdoc for $wp_admin_bar in admin-bar.php

Props jeremyfelt
see #24900

comment:5 @SergeyBiryukov18 months ago

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

comment:6 follow-ups: @toscho18 months ago

  • Cc info@… added
  • Resolution fixed deleted
  • Status changed from closed to reopened

These doc blocks are wrong: no function in this file requires an instance of WP_Admin_Bar. To make the doc blocks true, we have to add a type hint like this:

function wp_admin_bar_search_menu( WP_Admin_Bar $wp_admin_bar ) {

Currently, these functions accept any class with a matching set of public methods, no matter what they do. Doc blocks should never lie, this is sloppy code.

comment:7 in reply to: ↑ 6 @SergeyBiryukov18 months ago

Replying to toscho:

Currently, these functions accept any class with a matching set of public methods, no matter what they do.

Isn't it the same for functions that accept WP_Post, WP_User, or WP_Screen (see ticket:24992:2)?.

comment:8 in reply to: ↑ 6 @nacin18 months ago

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

Replying to toscho:

These doc blocks are wrong: no function in this file requires an instance of WP_Admin_Bar. To make the doc blocks true, we have to add a type hint like this:

function wp_admin_bar_search_menu( WP_Admin_Bar $wp_admin_bar ) {

Currently, these functions accept any class with a matching set of public methods, no matter what they do. Doc blocks should never lie, this is sloppy code.

I disagree. We cannot type-hint here for historical reasons. But even if we could, I still disagree. Of all the PHPDoc standards I can find, none say they must match a type-hint. The ongoing FIG standards specifically say "When provided it MUST contain a "Type" to indicate what is expected". Expected. Not enforced by a type-hint, just expected.

I'm totally fine with doc blocks "lying" in order to documented expected behavior. In my opinion, "these functions accept any class with a matching set of public methods" is rrelevant and invalid.

comment:9 @nacin18 months ago

Further: In the latest version of PHP, you can only type-hint certain non-scalar structures, like objects, arrays, and interfaces (also callable). The language is by its very nature a loosely typed language. @param bool $strict Means that $strict should be a boolean. But we can't enforce that in a type-hint, and frankly we don't care in this case as long as it is truthy or falsy.

We can code defensively to a point, but at some point, documentation can and will be more strict than the actual code.

Note: See TracTickets for help on using tickets.