Opened 12 years ago
Closed 12 years ago
#25450 closed defect (bug) (fixed)
remove_role() - invalid return statement
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 3.7 | Priority: | normal |
| Severity: | trivial | Version: | 2.0 |
| Component: | Inline Docs | Keywords: | has-patch |
| Focuses: | Cc: |
Description
Returns WP_Roles::remove_role(), which is void.
To omit the return looks more correct.
Attachments (1)
Change History (7)
#2
in reply to:
↑ 1
@
12 years ago
Replying to GaryJ:
@return void is described as very valid here:
http://www.phpdoc.org/docs/latest/for-users/phpdoc/types.html
There could be cases when we might need to distinguish between @return null and void. Void, of course, is null in reality.
But - I do not have any problem omitting @return void.
#3
follow-up:
↓ 4
@
12 years ago
I would rather omit @return null and @return void. To me, this indicates a deliberate decision, versus the function just not returning anything at the moment. Over time we have added return values to functions that didn't have them previously. For example, true/false on success.
#4
in reply to:
↑ 3
@
12 years ago
Replying to nacin:
In any case, returns should be consistent. It's pretty ugly when a function that should return a string, suddenly in the middle has if(...) return;
Even those methods having $echo = true parameters, in my opinion, should return something if echoed.
if ( $echo ) echo $var; // returned null, inconsistent else return $var;
I'd write as:
if ( $echo ) echo $var; return $var; // consistent, whether you want to use it or not
Anyway, what about my patch? Approved, in general? Because I can submit more like this. Please let me know. Thank you!
Looks sensible. No other uses of the remove_role() method or function call in core, and you're right in that the method that the function wraps doesn't return anything anyway.
As a side note about documentation, the use of
@return voidis no longer part of the inline documentation standard (since the discussion a few days ago), so that@returntag could be removed completely if thereturnkeyword is indeed going to be removed as well.