[wp-trac] [WordPress Trac] #58898: Fix WP_Text_Diff_Renderer_Table magic methods for PHP 8.2 dynamic properties
WordPress Trac
noreply at wordpress.org
Mon Jul 24 22:34:28 UTC 2023
#58898: Fix WP_Text_Diff_Renderer_Table magic methods for PHP 8.2 dynamic
properties
--------------------------+------------------------------------------------
Reporter: | Owner: hellofromTonya
antonvlasenko |
Type: defect (bug) | Status: assigned
Priority: normal | Milestone: 6.4
Component: | Version: 4.0
Administration |
Severity: normal | Keywords: needs-patch needs-unit-tests php82
Focuses: |
--------------------------+------------------------------------------------
`WP_Text_Diff_Renderer_Table` has a list of compatible fields
(`compat_fields`), which was introduced at the same time as the magic
methods. The fields in `compat_fields` list are then not dynamic
properties, as the magic methods properly handle each. But fields not in
that list are dynamic properties.
What happens if a field is not in the list?
* `__get()` falls through the method.
* `__set()` nothing happens as it falls through the method.
* `__isset()` returns `false`, which is correct.
* `__unset()` nothing happens as it falls through the method.
The problem:
When a dynamic property is called on this class, there's no information
given to the developer. The notice of `_doing_it_wrong()` will provide the
alert to developers to modify their code.
Also notice: the `compat_fields` are essentially public properties through
the magic methods.
=== Proposed Solutions
As proposed in #58896, 2 ways to fix this class to be compatible with PHP
8.2's dynamic properties:
1. **Option 1**: Fix the magic methods:
* Change 1: Add a notice or `_doing_it_wrong()` to each magic method, i.e.
to alert and inform developers when attempting to get/set/isset/unset a
dynamic property.
* Change 2: Add `return null` in `__get()` when attempting to get a
dynamic property that is not in the `compat_fields`.
2. **Option 2**: Add each `compat_fields` as a `public` property and
remove the magic methods.
Props to @jrf for identifying the issue.
References:
* These changes were
[https://www.youtube.com/live/vDZWepDQQVE?feature=share&t=10097 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.
--
Ticket URL: <https://core.trac.wordpress.org/ticket/58898>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform
More information about the wp-trac
mailing list