WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 years ago

#38630 new enhancement

Discourage usage of legacy properties in WP_Site

Reported by: flixos90 Owned by:
Milestone: Future Release Priority: low
Severity: normal Version: 4.5
Component: Networks and Sites Keywords: has-patch 2nd-opinion
Focuses: multisite Cc:
PR Number:

Description

Working on #38597, it was figured out that the best solution for handling problems with IDE handling of WP_Networks magic ID property is to rename the actual properties to reflect our current naming conventions. This enhancement will encourage to use the new conventions while still supporting the old ones for legacy code.

Let's do the same for WP_Site:

  • $blog_id (string) is replaced with $id (int)
  • $site_id (string) is replaced with $network_id (int)
  • both legacy names will continue to work through magic methods

Attachments (3)

38630.diff (2.6 KB) - added by flixos90 3 years ago.
38630.2.diff (3.1 KB) - added by flixos90 3 years ago.
38630.3.diff (3.3 KB) - added by flixos90 3 years ago.

Download all attachments as: .zip

Change History (14)

@flixos90
3 years ago

#1 @flixos90
3 years ago

  • Keywords has-patch added; needs-patch removed

38630.diff takes care of the proposed steps.

#2 @johnjamesjacoby
3 years ago

  • Version set to 4.5

I reviewed and approve of the proposed changes. Nice work!

#3 @flixos90
3 years ago

  • Keywords early removed
  • Milestone changed from Future Release to 4.8

@flixos90
3 years ago

#4 @flixos90
3 years ago

  • Keywords 2nd-opinion added

In 38630.2.diff I adjusted the to_array() method to be backward-compatible, but at the same time deprecated it.

The method is (as far as I can see) only called in wp_get_sites() which is already deprecated, and it is easily possible to achieve the same effect (only with the new properties instead) by using get_object_vars( $site ). We should probably discuss this change soon (as deprecations should happen early in the cycle), and it would definitely need to be highlighted in a multisite dev note.

If we decide deprecating it is right, let's do that in a separate ticket though.

Last edited 3 years ago by flixos90 (previous) (diff)

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


3 years ago

@flixos90
3 years ago

#6 @flixos90
3 years ago

38630.3.diff brings a few improvements over the previous patch:

  • In the to_array() method, it is ensured that the legacy properties are strings.
  • The $id and $network_id properties are no longer handled in the __get() and __isset() methods since they are public anyway. They are however still part of __set() since this method is called manually in the constructor.

Looking at the unit tests, I'm wary of this change although I definitely like its approach. Most unit tests failures (there are several) are only a result of the deprecation of to_array() which we can easily adjust, but the other two failures are actual failures, so anyone using get_object_vars() on WP_Site objects or iterating over them would run into problems (see https://core.trac.wordpress.org/ticket/40180#comment:3 for background). I'm starting to think we should hold off changing the public properties, although I don't like to say it.

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


2 years ago

#11 @flixos90
2 years ago

  • Milestone changed from 4.8 to Future Release
  • Priority changed from normal to low

This is a developer-centric change that could easily cause unexpected issues. We need to put more thoughts into it and merge it early in a major release. Let's come back to this later.

Note: See TracTickets for help on using tickets.