#24900 closed defect (bug) (fixed)
PHPDocs for $wp_admin_bar in admin-bar.php
Reported by: |
|
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)
Change History (13)
#2
@
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.
#3
@
12 years ago
Well then, there's that. :) Refreshed patch 24900.4.diff gets the order right and ditches the period.
#6
follow-ups:
↓ 7
↓ 8
@
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
@
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
@
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
@
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.
Previous patch included non relevant changes in other file