Make WordPress Core

Opened 4 weeks ago

Last modified 4 weeks ago

#63558 new enhancement

Proposal: Add `: void` return types to existing method signatures

Reported by: justlevine's profile justlevine Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch
Focuses: Cc:

Description

This ticket proposes implementing explicit : void return type declarations to existing WordPress method and functions.

Background

Type declaration usage has the following restrictions based on the minimum required PHP version of an application, whether it is WordPress Core, a plugin or a theme:
[...]

  • The iterable and void type declarations cannot be used until the minimum PHP version is PHP 7.1 or higher. The void type can only be used as a return type.

@return: Should contain all possible return types and a description for each. Use a period at the end. Note: @return void should not be used outside the default bundled themes and the PHP compatibility shims included in WordPress Core.

Code refactoring should not be done just because we can.

Justifications for refactor

  1. Minimum Requirements surpassed: WP's current minimum is 7.2+ (with 7.4 slated for 6.9)
  2. Improved Documentation: Unlike other return types, we don't note this in the doc-block. Adding it to the method both documents the @void with no extra lines. Plus, unlike a PHPDoc block, method signatures can't be mistaken or fall out of date
  3. Improved Developer Experience:
    • IDEs and tooling (including AI) can provide better autocomplete, error detection, and most-importantly type-safety guarantees - benefiting both extenders and core contributors.
    • Helps us with work on Codebase modernization and implementing PHPStan
  4. Negligible risk of invalidating other work: Since we don't refactor unnecessarily and generally avoid breaking changes, there isn't really much that can cause a diff conflict on a method signature - especially if we follow some guidelines.

Proposed scope/strategy

  1. Only target: private and final methods, and global functions - since the signature change won't break compatibility.
  2. Only update the signature to add a : void return type. While many of the arguments made in this ticket can be used to justify updating other return types, due to current lack of codebase type-safety these are much harder to verify.
  3. Only implement on methods that should actually return void. There's a lot of preexisting null/void coercion that will easier to untangle once our type safety efforts are further along.
  4. Don't update the method contents. Same reasons as the previous point.
  5. Make the changes across multiple PRs. This is more about making sure the PRs get merged quickly, than potential conflicts from other PRs. The longer it takes between creation and merge for a particular file or class, the more likely new methods may be added to that file that should also have a void return signature, but if a contributor sees other methods using return types, they'll be more likely to add them to their new code, saving some work down the line.

Next Steps

  1. I'm going to make a PR targeting a Class or two based on the above, that can serve as proof of concept - and assuming this proposal is accepted hopefully get it merged both for here and as part of our PHPStan efforts. Will wait for that acceptance before spending time updating other classes and encouraging others to join on the refactor effort.
  2. While we shouldn't "enforce it" yet on new code (iirc WPCS doesn't let you allow-list tech-debt, just manually ignore), we should write a Make post to encourage contributors/reviewers encouraging them to start using void return types.
  3. A future conversation about updating WPCS to enforce the use of : void when appropriate can happen at any point. It can be enforced on new code alone with PHPStan once merge (baselining methods we haven't updated yet), and PHPCS can autofix on once the existing codebase has been audited/updated. (PHPCS relies on the phpdoc type which as noted above isnt accurate in many places).

Change History (3)

#1 @justlevine
4 weeks ago

  • Summary changed from Code Quality: Add `: void` return types to non-breaking method signatures to Proposal: Add `: void` return types to existing method signatures

This ticket was mentioned in Slack in #core-coding-standards by justlevine. View the logs.


4 weeks ago

This ticket was mentioned in PR #8965 on WordPress/wordpress-develop by @justlevine.


4 weeks ago
#3

  • Keywords has-patch added

This PR updates the method signatures of several functions and files to enforce a : void return type. Changes are non-breaking and unlikely to disrupt other open PRs.

More specifically:

  • wp-signup.php : an example of global functions (cant be overwritten)
  • WP_Posts_List_Table : an example of private methods (don't get inherited)
  • WP_Screen : an example of methods on a final class (cant be extended)

While surfaced and part of PHPStan efforts (https://core.trac.wordpress.org/ticket/61175 / #7619 ), this PR serves as proof of concept for a larger proposal that would see : void return types implemented across the codebase in a nondisruptive manner while lay the groundwork for safely implementing other return types to existing method signatures.

Trac ticket: https://core.trac.wordpress.org/ticket/63558

Note: See TracTickets for help on using tickets.