Make WordPress Core

Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#24900 closed defect (bug) (fixed)

PHPDocs for $wp_admin_bar in admin-bar.php

Reported by: jeremyfelt's profile 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 12 years ago.
24900.2.diff (3.3 KB) - added by jeremyfelt 12 years ago.
Previous patch included non relevant changes in other file
24900.3.diff (3.7 KB) - added by DrewAPicture 12 years ago.
formatting
24900.4.diff (3.7 KB) - added by jeremyfelt 12 years ago.
Correct order for @param and @var docs to type, then name

Download all attachments as: .zip

Change History (13)

@jeremyfelt
12 years ago

@jeremyfelt
12 years ago

Previous patch included non relevant changes in other file

@DrewAPicture
12 years ago

formatting

#1 @DrewAPicture
12 years ago

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

#2 @nacin
12 years 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.

@jeremyfelt
12 years ago

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

#3 @jeremyfelt
12 years ago

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

Last edited 12 years ago by jeremyfelt (previous) (diff)

#4 @ryan
12 years ago

In 24999:

phpdoc for $wp_admin_bar in admin-bar.php

Props jeremyfelt
see #24900

#5 @SergeyBiryukov
11 years ago

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

#6 follow-ups: @toscho
11 years 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.

#7 in reply to: ↑ 6 @SergeyBiryukov
11 years 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)?.

#8 in reply to: ↑ 6 @nacin
11 years 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.

#9 @nacin
11 years 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.