Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#27098 closed defect (bug) (fixed)

Bundled Themes: ditch all uses of `@return void`

Reported by: philiparthurmoore's profile philiparthurmoore Owned by:
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.9
Component: Bundled Theme Keywords: has-patch
Focuses: docs Cc:

Description

It appears that the use of @return void should not be used for method or function DocBlocks:

What are your thoughts about removing this altogether?

Attachments (1)

ditch-return-void.patch (6.7 KB) - added by philiparthurmoore 11 years ago.

Download all attachments as: .zip

Change History (25)

#1 @obenland
11 years ago

In phpDocumentor:

It is RECOMMENDED when documenting to use this tag with every function and method. Exceptions to this recommendation are […] functions and methods without a return value, the @return tag MAY be omitted here, in which case @return void is implied.

So we're just being explicit here.

#2 @philiparthurmoore
11 years ago

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

Cool beans.

#3 @TobiasBg
11 years ago

  • Milestone Awaiting Review deleted

#4 follow-up: @DrewAPicture
11 years ago

  • Milestone set to Awaiting Review
  • Resolution invalid deleted
  • Status changed from closed to reopened

Re-opening for more discussion.

The core inline docs standard -- which is our first-and-foremost reference here, doesn't recommend using @return void anywhere outside of the bundled themes.

When we developed the standard last fall, the decision was made to ignore the use of @return void in the bundled themes, as they had been utilizing that style since Twenty Ten. The @return void tags aren't used in any consistency anywhere else in Core.

I think there's a good argument in favor of removing them altogether from the bundled themes -- this would bring that code in-line with the core standard.

Side note: It's important to remember that while the core inline documentation standard is largely based on and inspired by the phpDocumenter spec, it doesn't adhere to it 100 percent. Our standard is largely based on existing practices -- regardless of whether they might be considered "good" or "bad".

#5 in reply to: ↑ 4 @GaryJ
11 years ago

Replying to DrewAPicture:

When we developed the standard last fall, the decision was made to ignore the use of @return void in the bundled themes, as they had been utilizing that style since Twenty Ten.

The decision reference: http://irclogs.wordpress.org/chanlog.php?channel=wordpress-sfd&day=2013-09-25&sort=asc#m48668

I think there's a good argument in favor of removing them altogether from the bundled themes -- this would bring that code in-line with the core standard.

+1.

Side note: It's important to remember that while the core inline documentation standard is largely based on and inspired by the phpDocumenter spec, it doesn't adhere to it 100 percent.

Also to add, that some of the recent changes are based on the draft PSR-5, of which many code editors and phpDocumentor itself doesn't follow fully yet.

#6 @nacin
11 years ago

I don't know if it's worth patching older themes for it, but in general I'd follow core.

The basic argument for omitting @return void, for what it's worth, is rooted in the fact that we're always backwards compatible. For us, changing the return value of a function is pretty much a no-no. However, I have no qualms with adding a return value to a function that didn't have one previously, and we've done that a number of times. To me, @return void "bakes in" that the function returns no value, making it explicit. It's also possible to have the opposite feeling: that @return void specifically tells developers to not rely on a return value, at least until further notice. You could see it both ways.

#7 @MikeHansenMe
11 years ago

  • Keywords has-patch added

#8 @lancewillett
11 years ago

  • Milestone changed from Awaiting Review to 3.9

#9 follow-up: @lancewillett
11 years ago

  • Keywords 2nd-opinion added

Any more opinions on this? If we remove from Twenty Fourteen we should remove from Ten to Thirteen, also.

#10 @obenland
11 years ago

I'm still in favor of keeping as is. At worst, it would be "overdocumented". We can omit it in new default themes.

#11 in reply to: ↑ 9 @DrewAPicture
11 years ago

  • Keywords 2nd-opinion removed

Replying to lancewillett:

Any more opinions on this? If we remove from Twenty Fourteen we should remove from Ten to Thirteen, also.

We made the exception in the inline docs standard for the whole of the default themes library. So if we remove them from one theme, we need to do them all.

Personally, I'd prefer we remove them from the entire library. Fewer exceptions make for less confusion down the road. It also solidifies the standard as the standard through and through.

This ticket was mentioned in IRC in #wordpress-themes by lancewillett. View the logs.


11 years ago

#13 @lancewillett
11 years ago

  • Keywords needs-refresh added
  • Summary changed from Twenty Fourteen: Ditch all uses of `@return void`. to Bundled Themes: ditch all uses of `@return void`

Let's remove from all default themes.

#14 @lancewillett
11 years ago

In 27595:

Twenty Fourteen: remove doc block comments for @return void. See #27098, props philiparthurmoore.

#15 @lancewillett
11 years ago

In 27597:

Twenty Eleven: remove doc block comments for @return void. See #27098.

#16 @lancewillett
11 years ago

In 27598:

Twenty Thirteen: remove doc block comments for @return void. See #27098.

#17 @lancewillett
11 years ago

In 27599:

Twenty Twelve: remove doc block comments for @return void. See #27098.

#18 @lancewillett
11 years ago

  • Keywords needs-refresh removed
  • Resolution set to fixed
  • Status changed from reopened to closed

#19 follow-ups: @jond3r
11 years ago

A possibly warranted use of @return void is in cases where a function may return multiple types (which is all too common).

An example that I struggled with myself and which actually got committed in [25697] (for the function _fix_attachment_links()) uses

@return void|int|WP_Error

In this case void is returned if nothing is fixed, 0 or WP_Error on update failure, and the post ID on update success. (One could argue that this is poor design, but thankfully the function is not meant for public access).

I we cannot use void in this example, we are left with @return mixed, which is much less descriptive.

When the only return is void (i.e. "nothing") there is not much hurt in omitting @return void, but maybe we can officially allow it in "mixed" situations?

#20 in reply to: ↑ 19 @jond3r
11 years ago

Replying to jond3r:

When the only return is void (i.e. "nothing") there is not much hurt in omitting @return void, but maybe we can officially allow it in "mixed" situations?

A recent example where void was used in a @return, specifically as

* @return bool|void

was in [27575]. So I think we can say that void is allowed in mixed cases.

#21 in reply to: ↑ 19 ; follow-up: @GaryJ
11 years ago

Replying to jond3r:

A possibly warranted use of @return void is in cases where a function may return multiple types (which is all too common).

If a return tag is present, then it's never void. It's either a standard type, or null, even when the null is not explicit e.g. return;.

An example that I struggled with myself and which actually got committed in [25697]

That was committed in June 2013, while the discussion and decision to tidy up on the voids was not done until September, so it's not a sound example of agreement. Likewise, it doesn't follow the now-agreed coding standards of using (previously optional) braces.

@return void|int|WP_Error

In this case void is returned if nothing is fixed.

No, null is returned.

"If no parameter is supplied, then the parentheses must be omitted and NULL will be returned." - http://www.php.net/manual/en/function.return.php

Null is also returned when there is no return control structure, but that's a different debate.

A recent example where void was used in a @return, specifically as

  • @return bool|void

was in [27575]. So I think we can say that void is allowed in mixed cases.

It's not 100% clearcut, but I'd say that documentation is wrong. There is at least one return (so it can't be void) in the function, just not for a particular path - so in that case, it returns bool|null.

#22 in reply to: ↑ 21 ; follow-up: @jond3r
11 years ago

Replying to GaryJ:

This is interesting.

When I read the documentation about the return statement on php.net (your link above), it does say that NULL is returned: both if no return statement is given, and for a return without an argument.

I have a background in C/C++, where this is not true: void is returned in those cases. In C/C++ void is very distinct from NULL (literally 'nothing' versus the pointer address NULL (often defined as the integer 0)). In PHP, NULL apparently is a special value distinct from the integer 0. I had somewhat casually assumed that PHP followed C/C++ in this regard. Apparently not.

On the other hand, I might be correct anyway about the documentation of the return value. Reading the documentation of phpDoc http://www.phpdoc.org/docs/latest/references/phpdoc/types.html , under the section 'Keyword': '9. void' and '10. null', which I actually had read and which I based my comments above on, it seems that @return void should be used either if no return statement is given or if return without an argument is used.

So the interesting point here is that phpDocumentor sets itself apart from php.net and more adheres to the standard of C/C++ (and Java for that matter).

#23 in reply to: ↑ 22 ; follow-up: @GaryJ
11 years ago

The key document to read is https://github.com/php-fig/fig-standards/pull/169 (the in-progress pull request for PSR-5). It too RECOMMENDS that @return void should be used when no return is present, but I have my own view on that: http://code.garyjones.co.uk/why-return-void-is-wrong .

The overridding factor here is historical precedent - while the rest of PHP might be using @return void (perhaps originating from those with a similar background as yours), WP was only doing so in limited circumstances and doing it inconsistently so the cleanest and semantically correct option was to not use it at all.

#24 in reply to: ↑ 23 @DrewAPicture
11 years ago

Replying to GaryJ:

The overridding factor here is historical precedent - while the rest of PHP might be using @return void (perhaps originating from those with a similar background as yours), WP was only doing so in limited circumstances and doing it inconsistently so the cleanest and semantically correct option was to not use it at all.

Yep. And with that, I'm going to suggest that anyone interested continue this conversation at the weekly inline docs chat, held on Wednesdays at 19:00 UTC in #wordpress-sfd. I'll be happy to add it to the agenda.

We're also open to discussions on the make/docs P2 from interested community members.

Note: See TracTickets for help on using tickets.