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

[antonvlasenko]

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:

• These changes were ​discussed and agreed to in a livestream by @jrf @markjaquith and @hellofromTonya.
• The changes are part of #56034, which is the larger Dynamic Properties proposal and initiative.

[antonvlasenko]

I'm working on a patch for this issue.

[antonvlasenko]

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

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

[desrosj]

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.

@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!

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

[peterwilsoncc]

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.

[peterwilsoncc]

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

[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.

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

[antonvlasenko]

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.

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

[antonvlasenko]

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

[antonvlasenko]

#57570 was marked as a duplicate.

[antonvlasenko]

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.

[antonvlasenko]

#56535 was marked as a duplicate.

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

[antonvlasenko]

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.

[ironprogrammer]

Updated description per comment:20.

[hellofromTonya]

Self-assigning for review.

[hellofromTonya]

​Re-listening to the livestream (ie where @jrf @markjaquith and I discussed these changes), 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.

## 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

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

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.

[hellofromTonya]

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.

[hellofromTonya]

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

[hellofromTonya]

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.

[hellofromTonya]

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:

• A ​live open public working session where these changes were discussed and agreed to.
• ​PHP RFC: Deprecate dynamic properties.

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

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

Committed via https://core.trac.wordpress.org/changeset/55580.

[costdev]

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

[oglekler]

@antonvlasenko can you please address the comments above?

[antonvlasenko]

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).

[audrasjb]

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