Refactor magic methods in WP_List_Table, WP_User_Query, WP_Text_Diff_Renderer_Table and WP_Admin_Bar classes #3535
Conversation
anton-vlasenko
force-pushed
the
fix/disallow-dynamic-properties-in-wp-list-table
branch
from
a164aed
to
f90602a
Compare
|
@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! |
Warn developers about it.
…ed in Tests_Admin_WpListTable::$compat_fields).
anton-vlasenko
force-pushed
the
fix/disallow-dynamic-properties-in-wp-list-table
branch
from
7493f22
to
a1dc1d8
Compare
anton-vlasenko
changed the title
|
|
anton-vlasenko
changed the title
__isset should always return a boolean value.
…hey not needed anymore.
Let's not remove the magic methods.
…ons. The firing order of dataProviders vs. set_up_before_class() seems to depend on the PHPUnit version.
anton-vlasenko
changed the title
It creates issues. WP_Admin_Bar::$menu property should be declared instead to avoid any class behavior inconsistency. For example, isset($wp_admin_bar->menu) will return false, but $wp_admin_bar::$menu returns a value.
It's safe to remove it as it doesn't break BC. See https://wpdirectory.net/search/01GC9XDX4EBTTDF7G8AJNYRCJW All the plugins check if it exists before using it. And it doesn't exist because this property was never defined in the __isset magic method.
…) are not used in the WP_Admin_Bar class.
…ies method. 2. Add dataprovider. 3. Add comments.
2. Rename parameters in the named dataset. 3. Update comments.
| __( 'The "%1$s" property isn\'t defined. Dynamic properties are deprecated in PHP 8.2 and above.' ), | ||
| $name | ||
| ), | ||
| '6.3.0' |
There was a problem hiding this comment.
Good catch.
Fixed in a9e364e
|
Hi there, thanks for the PR! |
| * @return string|array|void | ||
| */ | ||
| public function __get( $name ) { | ||
| switch ( $name ) { |
There was a problem hiding this comment.
I've researched if these properties are being used (including WP plugins), and it looks like they can be safely removed without breaking BC.
There was a problem hiding this comment.
@anton-vlasenko is there a search query you used to investigate plugins and themes? If no, how did you investigate usage?
There was a problem hiding this comment.
That's a good question, @hellofromtonya.
Unfortunately, I'm not writing down every search query that I use (if we are talking about wpdirectory).
I committed these changes sometime last year, so I can't recall.
The best I can do is to search again and post the queries here.
There was a problem hiding this comment.
I'm not writing down every search query that I use
I was wrong, @hellofromtonya . I added the search query to a commit message.
So, I've used these search queries to find usages of $proto and $menu properties:
https://wpdirectory.net/search/01GC9XDX4EBTTDF7G8AJNYRCJW
https://wpdirectory.net/search/01GSTP3YTZP7R6PSHAZ51YPQBD
I was not able to find any occurrences of WP_Admin_Bar::$proto.
As for the WP_Admin_Bar::$menu, there are some occurrences, but isset() is always used to check if the property exists. So it should be safe to remove the $menu property.
There was a problem hiding this comment.
@hellofromtonya
I've changed my mind and brought WP_Admin_Bar::$menu back.
There is no reliable way to check all the plugins and themes because there are too many search results.
https://wpdirectory.net/search/01GSTW1X69TBN8FH3SY7V8KPY5 returns only a few results, all of which seem good (meaning that WP_Admin_Bar::$menu can be removed).
But if I use slightly broader search criteria (https://wpdirectory.net/search/01GSTW68SM7MNRZD0Z9AVBH6R0) I get about 4800 results. It's unrealistic to check all or at least the most popular plugins/themes.
So, to stay safe, I'd prefer to keep the WP_Admin_Bar::$menu property for now.
At the same time, WP_Admin_Bar::$proto can be removed. It doesn't seem to be used anywhere.
There was a problem hiding this comment.
Thank you for the search queries @anton-vlasenko.
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
menuis being used, add it as a public property on the class with a default value ofarray().
/**
* 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 theprotoproperty. 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.
There was a problem hiding this comment.
I've just submitted a PR that implements the changes proposed in the @hellofromtonya 's comment above. Thanks!
#4133
2. Always return value in __get() as this is what is expected from that magic methods.
anton-vlasenko
changed the title
It's not possible (or very problematic) to find out whether other plugins use this property in a safe manner (i.e., checking if it exists with (isset) before using it).
|
Closing this PR. Instead each of these classes will get a separate PR and commit to encapsulate their contextual history and individual considerations. |
This PR aims to refactor magic methods in WP_List_Table, WP_User_Query, WP_Text_Diff_Renderer_Table, and WP_Admin_Bar classes to make them compatible with PHP 8.2. Also, it ensures that developers will get a warning when using dynamic properties on these classes.
Trac ticket: https://core.trac.wordpress.org/ticket/56876
Trac ticket: https://core.trac.wordpress.org/ticket/58896
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.