WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#25450 closed defect (bug) (fixed)

remove_role() - invalid return statement

Reported by: tivnet Owned by: SergeyBiryukov
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)

remove_role_invalid_return_of_void.patch (630 bytes) - added by tivnet 6 years ago.

Download all attachments as: .zip

Change History (7)

#1 follow-up: @GaryJ
6 years ago

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 void is no longer part of the inline documentation standard (since the discussion a few days ago), so that @return tag could be removed completely if the return keyword is indeed going to be removed as well.

#2 in reply to: ↑ 1 @tivnet
6 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: @nacin
6 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 @tivnet
6 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!

#5 @SergeyBiryukov
6 years ago

  • Component changed from Users to Inline Docs
  • Milestone changed from Awaiting Review to 3.7

Appears to be a copy/paste in [3249].

The patch looks good. If we're removing the value from the docs, omitting the return makes sense.

#6 @SergeyBiryukov
6 years ago

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

In 25653:

Remove inaccurate @return value from remove_role(). props tivnet. fixes #25450.

Note: See TracTickets for help on using tickets.