Make WordPress Core

Opened 8 months ago

Last modified 2 months ago

#60875 new defect (bug)

Handler proposal for known dynamic properties that are initialized and set late only when getting its value

Reported by: hellofromtonya's profile hellofromTonya Owned by:
Milestone: 6.8 Priority: normal
Severity: normal Version:
Component: General Keywords: php82
Focuses: php-compatibility Cc:

Description

As noted in #56034:

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0, though this last part is not 100% certain yet.

There are multiple instances of known dynamic properties that only get initialized and set when getting their value (i.e. which happens within the __get() magic method).

For example:

While [54133] added #[AllowDynamicProperties] attribute to these classes, that strategy is specifically for unknown dynamic properties and not for known like the above examples.

Strategy

These types of known dynamic properties were intentionally designed to get late / lazily initialized each time code gets the property's value. As this is by design, retaining it must be a priority to ensure not only BC but also to avoid breaking code that depends upon it.

The strategy:

  • Retain the design of each property that is late / lazily initialized.
  • Declare the property on the class as either private or protected visibility.
  • Add __isset() magic method to ensure isset() works correctly (it does not right now). This step is needed for functions like wp_list_pluck() (see #59774).
  • Add __set() and __unset() to have a full set of magic methods, but document both to indicate their behavior gets overwritten the next time getting the property's value.
  • Add wp_trigger_error() with E_USER_DEPRECATED to each of the magic methods for all unknown dynamic properties.

See it in action:

This ticket is to:

  • layout the architectural strategy.
  • identify all Core classes with this type of dynamic property.
  • implement the strategy for each of instance.

References:

Change History (8)

#1 @antonvlasenko
7 months ago

One thing I stumbled upon while working on a new PR that implements this proposal is the need to be careful when adding protected and private properties to existing classes that previously only contained public properties (e.g., didn't have any protected/private properties before).

PHP adds a null byte character to the names of protected and private properties when converting an object to an array, and the simplexml_load_string() function throws an error as it can't handle the null byte character.

This code snippet illustrates this behavior: https://3v4l.org/XQL4f#v8.3.6 (change the WP_Term::$data's visibility to private or protected to reproduce the issue).

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


6 months ago

#3 @hellofromTonya
6 months ago

  • Keywords early added
  • Milestone changed from 6.6 to 6.7

This ticket is an epic for an overall strategy with implementation tickets, like #58087, implementing the strategy for their specific class.

I think this kind of change needs a long soak time, i.e. early in a major. As 6.6 is late in its beta, punting this ticket to 6.7 and marking for early.

#4 @hellofromTonya
6 months ago

Individual tickets for this strategy effort:

  • #61154 for attributes dynamic property in WP_Block.
  • #58087 for data dynamic property in WP_Term.

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


2 months ago

#6 follow-up: @joemcgill
2 months ago

@hellofromTonya was just reviewing this in a scrub of early tickets for the 6.7 milestone and wonder, given that this is a tracking ticket, if it would be best to assign early to any sub-tasks that are being tracked here and remove the keyword from this tracking ticket if it is likely to roll across milestones?

In the meantime, I'll leave this with you to either to the next milestone when appropriate.

#7 in reply to: ↑ 6 @hellofromTonya
2 months ago

  • Keywords early removed

Replying to joemcgill:

@hellofromTonya was just reviewing this in a scrub of early tickets for the 6.7 milestone and wonder, given that this is a tracking ticket, if it would be best to assign early to any sub-tasks that are being tracked here and remove the keyword from this tracking ticket if it is likely to roll across milestones?

In the meantime, I'll leave this with you to either to the next milestone when appropriate.

Thanks for the ping @joemcgill. Thinking any implementation can benefit for a long soak time. That said, the early does indeed need to be applied to the sub-task tickets. Removing it from this strategy ticket.

#8 @hellofromTonya
2 months ago

  • Milestone changed from 6.7 to 6.8

I've moved both sub-task tickets to 6.8 and am targeting them for early (early as possible) Alpha cycle to give each the benefits of a long soak time.

Sub-task tickets:

Note: See TracTickets for help on using tickets.