[wp-trac] [WordPress Trac] #61890: Handle WP_Term dynamic properties for PHP 8.2

[WordPress Trac]

#61890: Handle WP_Term dynamic properties for PHP 8.2
-------------------------------------+-------------------------------------
 Reporter:  hellofromTonya           |       Owner:  (none)
     Type:  defect (bug)             |      Status:  new
 Priority:  normal                   |   Milestone:  Future Release
Component:  Taxonomy                 |     Version:  4.4
 Severity:  minor                    |  Resolution:
 Keywords:  php82 has-patch needs-   |     Focuses:  coding-standards, php-
  dev-note early needs-test-info     |  compatibility
  needs-unit-tests                   |
-------------------------------------+-------------------------------------
Changes (by SirLouen):

 * keywords:
     php82 has-patch has-unit-tests needs-testing needs-dev-note has-test-
     info early
     => php82 has-patch needs-dev-note early needs-test-info needs-unit-
     tests


Comment:

 == Combined Reproduction and Patch Test Report
 === Description
 🟠 This report validates that the issue can be reproduced but can't fully
 validate that the patch works

 === Environment
 - WordPress: 6.9-alpha-60093-src
 - PHP: 8.4.6
 - Server: nginx/1.27.5
 - Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.4.6)
 - Browser: Chrome 136.0.0.0
 - OS: Windows 10/11
 - Theme: Twenty Twenty 2.9
 - MU Plugins: None activated
 - Plugins:
   * Classic Editor 1.6.7
   * Test Reports 1.2.0
   * WP Term Tester 1.0.0

 === Reproduction Tests
 1. I've been playing around with the reproduction code provided by
 @hellofromTonya and adapted it as a WP Plugin here:
 https://github.com/SirLouen/wp-term-tester/blob/main/wp-term-tester.php

 === Expected Results
 1. Given that currently thing are working, and deprecation notices are
 still not appearing. We should only expect things working before and after
 the patch
 2. The only big difference is found in test
 `test_object_id_should_not_be_cached_with_term_object` because with
 current formula since dynamic properties will be replaced by direct
 assignations, all properties have been defined in the `WP_Term` forcing
 this test to fail

 === Actual Results
 1. ✅ Things work pre- and post-patch as expected
 2. ❌ I've failed to trigger this on my test, despite on removing
 `#[AllowDynamicProperties]`, deprecation warnings are there (needs more
 testing info to reproduce this)

 {{{#!php
 wp_trigger_error(
         __METHOD__,
         "The property `{$name}` is not declared. Getting a dynamic
 property is " .
         'deprecated since version 6.7.0! Instead, declare the property on
 the class.',
         E_USER_DEPRECATED
 );
 }}}

 === Additional Notes

 Overall, this approach seems to be solving the issue except for the test
 that I want to review a little (and the trouble with the triggered error)

 Originally, the assertion was first introduced in [34999] as
 `$this->assertFalse( isset( $term->object_id ) );` which is passing also

 Then it was changed in [51397] to introduce the new Assert, that
 technically was not the same  `$this->assertObjectNotHasAttribute(
 'object_id', $term );` and there are no explicit intentions neither in the
 changeset or the ticket, so it feels it was an unfortunate change that
 happened to work out of chance (or maybe the test was useless, I have not
 really looked through the full [34999] in detail).

 The only difference with the [First Approach patch
 https://github.com/WordPress/wordpress-develop/pull/7224] is that
 additional properties pop-in given the explicit declaration (including
 `object_id` in the list of the declared `WP_Term` properties, which is
 exactly why the test modified in the PR was failing), but they are not
 necessarily being used.

 This report resembles a lot to this one: #61154, but contrarily, this
 patch is not failing, not even with a little tweak including all the tests
 used in #61154, neither before and after the patch.

 My opinion about the utility of all, is that, despite the patch is
 passing, I find the trouble that we are already using
 `#[AllowDynamicProperties]` which is not technically wrong (for now), and
 the possible removal is not even definitive yet (maybe it will never
 happen, given the number of people in PHP that voted against full removal
 of Dynamic Properties).

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/61890#comment:38>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform