Clarify exceptions for using @return void

[felixarntz]

See https://core.trac.wordpress.org/ticket/59619 and WordPress/wordpress-develop#5484 (review): The current explanation that @return void should only be used outside of bundled themes is missing two other areas where that rule does not apply.

[jrfnl]

@felixarntz Thanks for the PR.

Is there a reason for this addition ? Any recent discussion ?

I believe "third party libraries" has no place here as the coding standards do not apply to third party code anyway (unless they would choose to use it), but other than that, I'm fine with adding "PHP compatibility shims".

[jrfnl]

As a side-note: I believe this rule should be changed/removed, but that's outside the scope of this PR.

[felixarntz]

@jrfnl

Is there a reason for this addition ? Any recent discussion ?

Nothing other than what I linked in the PR description. This is not about a discussion or change on what we should be doing, it is merely documenting what is the current situation already: All usage of @return void is either in bundled themes, bundled third-party libraries, or PHP compatibility shims.

WordPress core does not necessarily control bundled third-party code, therefore this exclusion is more or less mandatory. For example, the bundled libraries PHPMailer and Requests contain @return void.

[jrfnl]

WordPress core does not necessarily control bundled third-party code, therefore this exclusion is more or less mandatory. For example, the bundled libraries PHPMailer and Requests contain @return void.

I agree that, as WP bundles third party code in a non-industry standard way, it may not always be clear to contributors what is third party code. However, that still doesn't take anything away from the fact that third party code is always excluded from project specific coding standards, so mentioning it here is redundant and not appropriate.

If at all necessary and not explicit enough already, it could be made more explicit on the introductory pages for the coding standards, but the current change is not the place (as inlining such an explicit exclusion for third party code for one particular rule creates ambiguity as if other rules would apply to external libraries).

[felixarntz]

However, that still doesn't take anything away from the fact that third party code is always excluded from project specific coding standards, so mentioning it here is redundant and not appropriate.

I disagree that it is redundant. There were questions about this on the linked PR, so clearly this is not obvious, even if you or I know about it.

If at all necessary and not explicit enough already, it could be made more explicit on the introductory pages for the coding standards

That would work for me.

[afercia]

Chiming in hoping to unblock this conversation :)
To me, the main issue now is that there is a changeset in core, which is part of the history of the project, that explicitly mentions bundled themes, third-party libraries, and PHP compatibility shims. See https://core.trac.wordpress.org/changeset/56943.

However, the documentation only mentions bundled themes.

This inconsistency doesn't help reducing confusion. I see a few issues that should be clarified:

1. The documentation doesn't clarify why @return void should not be used.
2. The statement should not is different from must not. should not implies that in some cases, it can be used. It's a recommendation, not a strict rule.

As a side-note: I believe this rule should be changed/removed, but that's outside the scope of this PR.

That would help simplifying things. 'Always add a return notation' is a way more simple concept rather than 'use return sometimes but sometimes not'. I would vote for making things simpler and always require a return notation.

[felixarntz]

@afercia I agree with most of what you're saying. Except:

'Always add a return notation' is a way more simple concept rather than 'use return sometimes but sometimes not'. I would vote for making things simpler and always require a return notation.

I don't think requiring @return void is a good call here. Yes, it's consistent in that every function then would get a @return annotation, but @return void functions don't actually return anything, so it's arguable that that has to be documented explicitly. More importantly though, this would be a significant documentation change with much larger implications than what we're discussing here. Would we go over all WP core functions and add @return void to it? That doesn't seem realistic or a good use of our time, and then if we don't do it we have even more inconsistency than now.

I think we should clarify the documentation for the exceptions, then that should be good. I wouldn't overinterpret the "must" in the commit message. Commit messages don't go through any review and I just wrote that without thinking much about the difference. That message (like other commit messages) carries no authority on any documentation matters.

[jrfnl]

@afercia I agree with most of what you're saying. Except:

'Always add a return notation' is a way more simple concept rather than 'use return sometimes but sometimes not'. I would vote for making things simpler and always require a return notation.

I don't think requiring @return void is a good call here. Yes, it's consistent in that every function then would get a @return annotation, but @return void functions don't actually return anything, so it's arguable that that has to be documented explicitly. More importantly though, this would be a significant documentation change with much larger implications than what we're discussing here. Would we go over all WP core functions and add @return void to it? That doesn't seem realistic or a good use of our time, and then if we don't do it we have even more inconsistency than now.

I think we should clarify the documentation for the exceptions, then that should be good. I wouldn't overinterpret the "must" in the commit message. Commit messages don't go through any review and I just wrote that without thinking much about the difference. That message (like other commit messages) carries no authority on any documentation matters.

While I'd very much like a discussion on always requiring @return tags (and did an extensive write up about why we should some years back when an RFC repo opened, but then the repo was closed before I could open a PR), this is not the place for that discussion.

I have already said that the current PR is not acceptable and what should happen instead. The PR needs an update or it won't get merged.

[felixarntz]

@jrfnl

I have already said that the current PR is not acceptable and what should happen instead. The PR needs an update or it won't get merged.

I am unclear what should happen instead, I was waiting for a reply from you to my comment #133 (comment). Based on that, what do you think should happen?

[jrfnl]

@jrfnl

I have already said that the current PR is not acceptable and what should happen instead. The PR needs an update or it won't get merged.

I am unclear what should happen instead, I was waiting for a reply from you to my comment #133 (comment). Based on that, what do you think should happen?

I don't see a question there, only a "that would work for me", so I can but repeat myself:

If at all necessary and not explicit enough already, it could be made more explicit on the introductory pages for the coding standards, but the current change is not the place (as inlining such an explicit exclusion for third party code for one particular rule creates ambiguity as if other rules would apply to external libraries).

[felixarntz]

@jrfnl PR updated.

[jrfnl]

Suggested change

	The WordPress Coding Standards apply to WordPress code. Third-party libraries are not subject to these standards, even when bundled with the main project code. This may occasionally be the case, for example WordPress core bundles several third-party libraries in its codebase.
	Third-party libraries are not subject to these standards, even when bundled with the main project code. This may occasionally be the case, for example: WordPress core bundles several third-party libraries in its codebase.

The first bit creates ambiguity. What is "WordPress code" ? Core ? plugins ? themes ? closed source company specific site configuration ?

What's important is that third-party libraries are not subject to the standards, so removing the first phrase does not take anything away from the message you are trying to get across.

You may want to rethink the <h2> title though, once the first phrase has been removed.

[felixarntz]

@jrfnl Updated.

[jrfnl]

Thanks for the updating the PR @felixarntz . It will be merged after a second review.

[dingo-d]

Left one suggestion, just reads a bit better for me.

[dingo-d]

Suggested change

	<h2>Where to (not) apply the coding standards</h2>
	<h2>Where not to apply the coding standards?</h2>

This reads better considering the paragraph below in my opinion.

[GaryJones]

Suggested change

	<h2>Where to (not) apply the coding standards</h2>
	<h2>Where should the coding standards not apply?</h2>

[felixarntz]

@dingo-d The "not to" makes sense, but why a question mark? This heading is not a question.

[dingo-d]

To me, with the paragraph below explaining where not to apply them, it reads more like a question (plus where is a question word).

The heading doesn't read with where as a conjunction or a relative pronoun.

[felixarntz]

It's not a question though. For example, the common heading "How to do xyz" also starts with a question word, but it's not a question.

Generally, most headings aren't sentences, including this one, so unless we make it an actual question (e.g. "Where should the coding standards not be applied?"), it's not a question either.

The way the words "Where" and "How" are used in this kind of heading is probably closest to "{This section describes...} Where not to apply the coding standards". So I'd argue that "where" is closest to a conjunction here.

[felixarntz]

I now see your suggestion here @GaryJones. That one could make sense as a question.

I don't really see the benefit as the "Where to ..." is a pretty common pattern for headings, but if you feel strongly, "Where should the coding standards not apply?" works for me. Or maybe: "Where do the coding standards not apply?" I'd argue it's that they "don't apply" rather than that they "should not apply".

[jrfnl]

I agree with the "don't apply".

[dingo-d]

Where do the coding standards don't apply? reads better to me 👍🏼

[felixarntz]

Updated to "Where do the coding standards not apply?"

[GaryJones]

Suggested change

	<h2>Where to (not) apply the coding standards</h2>
	<h2>Where should the coding standards not apply?</h2>

[GaryJones]

Suggested change

	Third-party libraries are not subject to these standards, even when bundled with the main project code. This may occasionally be the case, for example WordPress core bundles several third-party libraries in its codebase.
	Even when integrated with the primary project, third-party libraries are exempt from these standards. This includes instances like WordPress core, where multiple third-party libraries are incorporated into its codebase.

[felixarntz]

@GaryJones The 2nd sentence sounds great, but for the 1st one, I'm not sure about which benefits this would have over the current way. It just shifts main clause and sub clause as far as I can tell, which I think in this case makes for a worse order, since IMO:

• The main aspect to clarify is that third-party libraries are exempt, so that makes sense to come first.
• The "even when bundled" bit ties to the sentence that follows, so that makes sense to come second.

[felixarntz]

I've incorporated your suggestions without changing the main clause sub clause order in b0d5d5f.

[jrfnl]

LGTM, should "not" be made italic to draw more attention to it ?

[felixarntz]

@jrfnl Happy to make it italic if others agree? @GaryJones @dingo-d

[dingo-d]

Yup, go ahead 🙂

[felixarntz]

@dingo-d Done :)