Make WordPress Core

#56876 closed defect (bug) (fixed)

Dynamic Properties: Fix magic methods in WP_List_Table, WP_User_Query, WP_Text_Diff_Renderer_Table and WP_Admin_Bar classes

Reported by: antonvlasenko's profile antonvlasenko Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch has-unit-tests php82 early commit reporter-feedback
Focuses: Cc:

Description (last modified by costdev)

WP_List_Table, WP_User_Query, and WP_Text_Diff_Renderer_Table classes implement an incomplete set of magic methods.

But these magic methods need to be refactored to ensure that these classes are compatible with PHP 8.2 (the magic methods in these classes are only partially implemented, the missing/incomplete magic methods should be added/refactored).

Also, developers should get a warning when they try to set (get) dynamic properties on objects of these classes.

As for the WP_Admin_Bar class: an investigation is needed to decide whether WP_Admin_Bar::__get() can be removed.

Props to @jrf for identifying the issue .

References:

Change History (40)

#1 @antonvlasenko
18 months ago

I'm working on a patch for this issue.

#2 @SergeyBiryukov
18 months ago

  • Component changed from General to Administration

#3 @antonvlasenko
18 months ago

Would it be possible to add the "php82" keyword to this ticket?
Thanks.

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


18 months ago
#4

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

This PR aims to improve the compatibility of the WP_List_Table class with PHP 8.2 and above by ensuring that developers get a proper error notice when they try to set/get a dynamic property.
Before this PR, WordPress wouldn't display any warning about incorrect usage of dynamic properties.

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

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


18 months ago

#6 @desrosj
18 months ago

  • Keywords php82 added
  • Milestone changed from Awaiting Review to 6.1.1
  • Version trunk deleted

Though 8.2 aims to bring WordPress to a more compatible state with PHP 8.2, this is not a new issue in 6.1/trunk, so unsetting the Version field.

Let's take a look at this for 6.1.1 or 6.2, whichever the committers familiar with these efforts are more comfortable with.

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


18 months ago

@JeffPaul commented on PR #3535:


18 months ago
#8

@SergeyBiryukov @jrfnl pinging you here in hopes one of you can help give this a review and help it land in 6.1.1... thank you!

@JeffPaul commented on PR #3535:


18 months ago
#9

@hellofromtonya also pinging you as well, any help with review/commit on this for 6.1.1 would be appreciated... thank you!

#10 @peterwilsoncc
18 months ago

The linked pull request includes a new string, 'The "%1$s" property isn\'t defined. Dynamic properties are deprecated in PHP 8.2 and above.'.

I think 6.2 would be better, if possible, to avoid the need for new language packs.

#11 @peterwilsoncc
18 months ago

Nevermind, I realised 6.1.1 already includes a new string. As you were.

#12 follow-up: @desrosj
18 months ago

  • Milestone changed from 6.1.1 to 6.2

At first, I was a +1 to include this in 6.1.1. However, this addresses only 1 specific class of potentially many with this issue.

There is a really detailed plan for addressing this laid out in #56034, and I think that this should be evaluated within the context of that plan.

It's highly preferable that all of the classes with this problem get fixed at the same time.

Moving this to 6.2 to match the current milestone for #56034.

#13 in reply to: ↑ 12 @antonvlasenko
17 months ago

Replying to desrosj:

At first, I was a +1 to include this in 6.1.1. However, this addresses only 1 specific class of potentially many with this issue.

There is a really detailed plan for addressing this laid out in #56034, and I think that this should be evaluated within the context of that plan.

It's highly preferable that all of the classes with this problem get fixed at the same time.

It may be more convenient to have everything in one PR, but that PR is going to be pretty large (given that each class that is being fixed also needs unit tests).
Discussing fixes for each class can also take a long time, delaying the deployment of the fix.
So, from my point of view, it will be more convenient to release fixes gradually as they get approved.

@antonvlasenko commented on PR #3535:


15 months ago
#14

I want to add a fix for another class to this PR because the fixes are similar.
So please don't review for now.

#15 @antonvlasenko
15 months ago

  • Summary changed from WP_List_Table should prevent using dynamic properties as they are not allowed in PHP 8.2 to WP_List_Table and WP_User_Query should prevent using dynamic properties as they are not allowed in PHP 8.2

The PR is ready to be reviewed.
Sorry for spamming, but I couldn't find a way to edit my previous message.

Last edited 15 months ago by antonvlasenko (previous) (diff)

#16 @antonvlasenko
15 months ago

#57570 was marked as a duplicate.

#17 @antonvlasenko
15 months ago

  • Summary changed from WP_List_Table and WP_User_Query should prevent using dynamic properties as they are not allowed in PHP 8.2 to WP_List_Table, WP_User_Query, WP_Text_Diff_Renderer_Table classes should prevent using dynamic properties as they are not allowed in PHP 8.2

It's highly preferable that all of the classes with this problem get fixed at the same time.

I've closed https://core.trac.wordpress.org/ticket/57570 in favor of this task and cherry-picked all related changes to https://github.com/WordPress/wordpress-develop/pull/3535.

This ensures that all the classes with this problem get fixed simultaneously.

@desrosj Please let me know if I should also cherry-pick changes from https://core.trac.wordpress.org/ticket/56535 and close it in favor of this ticket.

#18 @antonvlasenko
15 months ago

#56535 was marked as a duplicate.

@audrasjb commented on PR #3535:


14 months ago
#19

Hi there, thanks for the PR!
I added a few very small change requests.

#20 @antonvlasenko
14 months ago

  • Summary changed from WP_List_Table, WP_User_Query, WP_Text_Diff_Renderer_Table classes should prevent using dynamic properties as they are not allowed in PHP 8.2 to Refactor magic methods in WP_List_Table, WP_User_Query, WP_Text_Diff_Renderer_Table and WP_Admin_Bar classes

Updated description for this ticket (I'm unable to change it with my .org account):
WP_List_Table, WP_User_Query, and WP_Text_Diff_Renderer_Table classes implement some magic methods.
But these magic methods need to be refactored to ensure that these classes are compatible with PHP 8.2 (the magic methods in these classes are only partially implemented, the missing/incomplete magic methods should be added/refactored).
Also, developers should get a warning when they try to set (get) dynamic properties on objects of these classes.
As for the WP_Admin_Bar class: an investigation is needed to decide whether WP_Admin_Bar::__get() can be removed.

#21 @ironprogrammer
14 months ago

  • Description modified (diff)

Updated description per comment:20.

#22 @hellofromTonya
14 months ago

  • Owner set to hellofromTonya
  • Status changed from new to reviewing

Self-assigning for review.

#23 @hellofromTonya
14 months ago

  • Keywords early added
  • Milestone changed from 6.2 to 6.3

Re-listening to the livestream, consensus was to commit these changes when the alpha branch is first opened. Why? To give the longest amount of time possible for feedback and reports of issues.

Punting this ticket to 6.3 early. The 6.3 alpha will happen after 6.2-RC1 which is coming fast (less than 2 weeks away). The plan is to finish the reviews and patches (1 patch per class) and then commit each once alpha is open.

Version 0, edited 14 months ago by hellofromTonya (next)

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


14 months ago
#24

## This is a draft PR. Please don't review.

This PR aims to refactor WP_Admin_Bar and remove the __get magic method. Magic methods are deprecated in PHP 8.2.

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

@hellofromTonya commented on PR #3535:


13 months ago
#25

Closing this PR. Instead each of these classes will get a separate PR and commit to encapsulate their contextual history and individual considerations.

@hellofromTonya commented on PR #4133:


13 months ago
#26

Context for these changes were shared here https://github.com/WordPress/wordpress-develop/pull/3535#discussion_r1115026361. Copying them here.

Reviewing the first query for menu shows that at least 1 plugin (s2 Member Framework) is using this property.

As discussed in the livestream, the plan is:

  • Remove the __get().
  • Since menu is being used, add it as a public property on the class with a default value of array().
	/**
	 * Deprecated menu property.
	 *
	 * @since 3.1.0
	 * @deprecated 3.3.0 Modify admin bar nodes with WP_Admin_Bar::get_node(), WP_Admin_Bar::add_node(), and WP_Admin_Bar::remove_node().
	 * @var array
	 */
	public $menu = array();
  • What about proto (for protocol) property?

From the livestream, there was consensus to remove it if _unused_, but with an action to research what the property is and why it was added.

History on the proto property:

  • r16038 added this property.
  • r19501 moved it from the initialize() method to __get().
  • When 3.1 released, the protocol handling switched to use get_admin_url(), admin_url(), etc functions instead of the proto property. But the property remained in the code, unused.

Would removing it be a BC break? If not used, then no. But if some plugin or theme in the wild uses it, then yes it is a BC break.

Let's remove it with this reasoning:

  • It's unused in Core and none found in wporg search
  • It was introduced in the 3.1 cycle but then replaced with URL get functions during that same cycle.
  • It's reasonable then to say that it should have been removed in 3.1, but was missed. It appears to be dead code.

#27 @hellofromTonya
13 months ago

  • Summary changed from Refactor magic methods in WP_List_Table, WP_User_Query, WP_Text_Diff_Renderer_Table and WP_Admin_Bar classes to Dynamic Properties: Fix magic methods in WP_List_Table, WP_User_Query, WP_Text_Diff_Renderer_Table and WP_Admin_Bar classes

#28 @hellofromTonya
13 months ago

  • Keywords commit added

Patch: https://github.com/WordPress/wordpress-develop/pull/4133

This patch:

  • removes WP_Admin_Bar::__get()
  • declares menu as a property on the class and initializes it to an empty array.
  • removes the unused proto dynamic property.

Why remove the proto?

tl;dr

  • This property was not intended to be released in 3.1.
  • There are no usages of it in WP.org's plugin or theme directory.
  • There's consensus from @markjaquith @jrf and me that it's safe to remove 'proto'.
  • Plan: Commit early to give time for reports of usages.

More Details:

The protocol handling was added during the 3.1 development cycle but its usage was removed and switched to use get_admin_url(), admin_url(), etc functions. WP 3.1.0 shipped with the property in the class, but not used in Core.

During the 3.1 development cycle:

  • [16038] added this property to the WP_Admin_Bar class and switched it to https:// if SSL in WP_Admin_Bar::initialize().
  • [16308] eliminated its usage in Core, but the property remained in WP_Admin_Bar.
  • It appears this property was missed in [16308] and should have been removed before 3.1 shipped.

In WP 3.3, [19501] / #19371 introduced the __get() magic method for BC of 'menu':
Source

making sure that $this->menu returns back compat code

Source

We're not going to maintain compat for $menu. Suggest we make it array() and plugins will have to deal. We can throw a _deprecated_argument() and push them to use the new methods.

Notice, there's no mention of usages or BC concerns for 'proto' property, but it too was added to the magic method.

Last edited 13 months ago by hellofromTonya (previous) (diff)

#29 @hellofromTonya
13 months ago

I'll let this sit for any further considerations. If none, then will commit on Monday, March 20th.

#30 @hellofromTonya
13 months ago

  • Description modified (diff)

Updated the ticket's description to include the references to:

  • the livestream, where these changes were discussed and agreed to by @markjaquith, @jrf, and me.
  • #56034 - as this change is part of that work.

#31 @hellofromTonya
13 months ago

  • Description modified (diff)

#32 @hellofromTonya
13 months ago

In 55580:

Code Modernization: Fix dynamic properties in WP_Admin_Bar.

To fix the dynamic properties, the following changes are included:

  • Removes WP_Admin_Bar::__get().
  • Declares menu as a property on the class, deprecates it, and initializes it to an empty array.
  • Removes the unused 'proto' dynamic property.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

Why remove the WP_Admin_Bar::__get() magic method?

tl;dr
The magic method is no longer needed.

The magic method only handled the menu and proto dynamic properties. Introducing a full set of magic methods is overkill for this class. Instead of having to maintain magic methods, this changeset instead directly addresses the 2 properties (see below).

Why declare the menu property on the class?

tl;dr
To simplify the code while maintaining backwards compatibility for extenders who are using this deprecated property.

The menu property was introduced during the 3.1.0 development cycle as a declared property [15671]. Its purpose was to internally hold the menu structure.

During the WP 3.3.0 development cycle, it was replaced by a new private property called nodes (see [19120]).

But breakage reports from extenders caused it to be restored. [19501] added the __get() magic method, i.e. for handling it as a dynamic property, and deprecated it.

We're not going to maintain compat for $menu. Suggest we make it array() and plugins will have to deal. We can throw a _deprecated_argument() and push them to use the new methods.

~ Source: see #19371 comment 17

A search of the wp.org plugins and themes repository shows that a few plugins are still using this deprecated property. To maintain backwards compatibility, menu is moved back to the class as a declared property, set to an empty array (as it's been since 3.3.0), and deprecated in the property's description.

Why remove the proto dynamic property?

tl;dr

  • It was not intended to be released in 3.1.
  • There are no usages of it in Core or in the WP.org's plugin or theme directories.
  • It should be safe to remove.

This property was first introduced in the WP 3.1.0 development cycle to replace the PROTO constant (see [16038]) for protocol handling for the admin bar's hyperlinks. [16077] replaced the property's usages with URL functions such as get_admin_url() and admin_url(). But it missed removing the property, which was no longer needed or used.

It was relocated to the __get() magic method as a dynamic property when the menu property was restored (see [19501]).

A search of WP.org's plugins and themes repositories shows no usages of the property. Core hasn't used it since the removed in [16038] before 3.1 final release. It should be safe to remove it, but committing very early in the 6.3 alpha cycle to give time for reports of usages, if there are any.

References:

Follow-up to [19501], [19120], [16308], [16038], [15671].

Props antonvlasenko, hellofromTonya, jrf, markjaquith, desrosj, ironprogrammer, peterwilsoncc, SergeyBiryukov.
See #56876, #56034.

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


11 months ago

#35 @costdev
11 months ago

  • Description modified (diff)
  • Keywords reporter-feedback added

This ticket was discussed in the recent bug scrub. It was felt that splitting this into separate tickets would help each move forward and for the respective ticket to be closed in the appropriate milestone (and possibly component).

@antonvlasenko Are you happy to handle the creation of separate tickets? And is this one ready to rename and close for WP_Admin_Bar?

Additional props: @audrasjb @SergeyBiryukov

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


11 months ago

#37 @oglekler
10 months ago

@antonvlasenko can you please address the comments above?

#38 @antonvlasenko
10 months ago

Can you please address the comments above?

Thanks for the ping, @oglekler!
Somehow I missed the message above. My apologies.
I will be happy to split this task into separate ones as soon as I have the opportunity (probably later this week).

Last edited 10 months ago by antonvlasenko (previous) (diff)

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


10 months ago

#40 @audrasjb
10 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

Thanks @antonvlasenko!
As per today's bug scrub, I'm therefore closing this as fixed.

Note: See TracTickets for help on using tickets.