Make WordPress Core

Opened 9 months ago

Last modified 7 days ago

#59232 new enhancement

Introduce #[Override] attribute to mark overloaded methods

Reported by: jrf's profile jrf Owned by:
Milestone: 6.6 Priority: normal
Severity: normal Version: 6.4
Component: General Keywords: php83 has-patch
Focuses: Cc:

Description

From https://core.trac.wordpress.org/ticket/59231:

Marking overridden methods

This is a new feature which will be introduced in PHP 8.3, with limited validation functionality attached. The attribute basically allows to mark methods in a (child) class/interface which overload a method in a parent class or from an interface, as doing so intentionally.

Per the RFC:

... being able to express if a method is intended to override another method or implement an interface would make it easier to debug a mistake, to refactor and to clean up existing code. Another possible use case is to easily detect a possibly breaking change in a parent class that was provided by a library without needing to read the changelog in detail or missing some item in the list of changes

I'd like to advocate for adding these attributes to WP Core in all the relevant places as it:

  • Increases awareness of the method overload for contributors.
  • Can serve as a warning that the method signature should not be touched (unless the parent method signature changes).
  • Has no downside as attributes are ignored in older PHP versions and in PHP versions where the attribute referenced does not exist.

In the rare case that the attribute, once added, would result in a fatal error, that would be fantastic, as that means we have actually found a bug in WP before it got into a stable release.

Change History (12)

#1 @jrf
9 months ago

  • Keywords needs-patch added

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


9 months ago
#2

  • Keywords has-patch added; needs-patch removed

#3 @SergeyBiryukov
8 months ago

  • Milestone changed from Awaiting Review to 6.4

@jrf commented on PR #5166:


8 months ago
#4

Awesome work team!

I haven't reviewed/verified the patch in detail, but just thought I'd leave a comment about something to think about.

The attribute as used in this patch - #[\Override] - is using a fully qualified class name.

In non-namespaced files (as all WP Core files are), it could be elected to not use the leading backslash.
Alternatively, a use Override import statement could be used.

I am in favour of using the fully qualified names, as is done in this patch, as it ensures that if, at some hypothetical time in the future, namespaces would be introduced, this code will continue to work without the need for further changes.

However, the other attributes currently in use in WP Core - #[AllowDynamicProperties] and #[ReturnTypeWillChange] - are used in an unqualified form.

I wonder if we should make all attributes consistently use fully qualified names ?

Note: this doesn't need to be addressed in this PR, but is a question which the changes in this PR triggered for me and something I believe we should have a think about.

This ticket was mentioned in Slack in #core by oglekler. View the logs.


8 months ago

#6 @oglekler
8 months ago

This ticket was discussed during bug scrub.

Hi @SergeyBiryukov this looks like a straight forward task, but we need another ticket to make things consistent :) Can you make this? Thank you 🙏

Add props to @mukesh27

This ticket was mentioned in Slack in #core by oglekler. View the logs.


8 months ago

#8 follow-up: @hellofromTonya
8 months ago

  • Type changed from enhancement to task (blessed)

The changes with this ticket will not impact production code. Rather, they are adding an annotation necessary for PHP 8.3 compatibility. Thus, I've changed the ticket to a task and it can go into the 6.4 beta cycle.

#9 in reply to: ↑ 8 @jrf
8 months ago

Replying to hellofromTonya:

The changes with this ticket will not impact production code. Rather, they are adding an annotation necessary for PHP 8.3 compatibility. Thus, I've changed the ticket to a task and it can go into the 6.4 beta cycle.

I'm happy for this to stay in the 6.4 beta cycle, but it is not a necessity. This is not needed for PHP 8.3 compatibility. It is a ticket to take advantage of a new PHP 8.3 feature which can help prevent future coding mistakes.

#10 @hellofromTonya
7 months ago

  • Milestone changed from 6.4 to 6.5
  • Type changed from task (blessed) to enhancement

Thanks for clarifying @jrf :)

The PR is still in draft mode. With 6.4 RC1 in less than 15 hours, moving this to 6.5 and changing it back to an enhancement.

#11 @swissspidy
3 months ago

  • Milestone changed from 6.5 to 6.6

#12 @oglekler
7 days ago

@SergeyBiryukov what is the decision about consistency of attributes? It looks simple enough to move this forward without many complications.

Note: See TracTickets for help on using tickets.