Fix the 'attributes' dynamic property in WP_Block

[antonvlasenko]

The WP_Block class employs the __get magic method to compute the attributes class property.
However, since PHP 8.2 does not support dynamic properties, it is better to eliminate this approach and explicitly declare the WP_Block::$attributes class property to store the block's attributes instead.

[antonvlasenko]

I'm working on a patch for this ticket.

Trac ticket: https://core.trac.wordpress.org/ticket/61154

[antonvlasenko]

I've just submitted ​https://github.com/WordPress/wordpress-develop/pull/6500
This PR:

1. Fixes the WP_Block::$attributes dynamic property by utilizing strategy proposed by @hellofromTonya in https://core.trac.wordpress.org/ticket/60875. Props @hellofromTonya.
2. Implements this suggestion.
3. Adds unit tests to check how the newly declared magic methods on the WP_Block class handle the WP_Block::$attributes property and other declared public properties.

[ironprogrammer]

Thank you for the PR, @antonvlasenko! 🙌🏻 I've tagged this for ​PHP 8.2 compat, and moved it to the 6.6 milestone to get it on the radar.

[faisal03]

== Test Report

Please ignore, it was meant for https://core.trac.wordpress.org/ticket/60236

[hmbashar]

How can I test the issue?

[hellofromTonya]

Related #60875.

This ticket is part of the epic #60875 for handling late initialization and setting of dynamic properties.

As this effort is now slated for 6.7, moving this ticket too.

[antonvlasenko]

Replying to hmbashar:

How can I test the issue?

That's an excellent question, @hmbashar.

It is somewhat challenging to outline precise steps for testing this, as it primarily involves refactoring.
Ideally, PHPUnit tests that have been added in my PR should ensure that the WP_Block class functions correctly.

Here's my best approach for testing this PR:

1. Review the code changes and assess whether the architecture is fine.
2. Ensure GitHub CI jobs pass.
3. Use the Site Editor. Test adding, updating and deleting blocks, ensuring there are no WP_Block related errors in the error log.

[hellofromTonya]

IMO this kind of change will benefit from a long soak time (to help maximize the opportunities for feedback, testing, and any needed follow-ups), either by first releasing it in the Gutenberg plugin and/or committing it early during a major's Alpha cycle.

Also, thinking more time is needed to discover and understand its contextual history as well as if / how this specific dynamic property is still being used in Core.

As 6.7 Beta 1 is a few days, early Alpha has passed. Moving this ticket to the next major. That said, let's take the time between now and early 6.8 Alpha to solidify a solution so that it's ready to be committed once alpha is open.

@hellofromtonya Thank you for the thorough review!
I've replied to your comments, and I’m happy to provide any further clarification or updates if needed.
Please let me know if you’d like me to expand on anything or if additional feedback is required.

@hellofromtonya Thank you for the thorough review!
I've replied to your comments, and I’m happy to provide any further clarification or updates if needed.
Please let me know if you’d like me to expand on anything or if additional feedback is required.

@anton-vlasenko I've taken the liberty of merging trunk in to your PR in order to retrigger the test run so I can see the logs. Unfortunately Tonya is taking a break from contributing so I will try to pick up the review in order to get the PR in to 6.8.

This change is causing the following test to fail as the __default attribute is not replaced.

​https://github.com/WordPress/wordpress-develop/blob/d9cb6e7e9de61e36ba286aafcacc5d6dba2fc559/tests/phpunit/tests/block-bindings/render.php#L336-L372

There was 1 failure:

1) WP_Block_Bindings_Render::test_default_binding_for_pattern_overrides
The `__default` attribute should be replaced with the real attribute prior to the callback.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<p>This is the content value</p>'
+'<p>This should not appear</p>'

/var/www/tests/phpunit/tests/block-bindings/render.php:358
/var/www/vendor/bin/phpunit:122

[audrasjb]

As per today's bug scrub: thanks for the PR @antonvlasenko!
@peterwilsoncc merged trunk to re-launch unit tests and it appears we have one failing test.
​https://github.com/WordPress/wordpress-develop/pull/6500#issuecomment-2601065023

Could you please refresh the patch so we can commit this early in the release cyle? Thank you!

[audrasjb]

Self assigning for review.

This change is causing the following test to fail as the __default attribute is not replaced.

​https://github.com/WordPress/wordpress-develop/blob/d9cb6e7e9de61e36ba286aafcacc5d6dba2fc559/tests/phpunit/tests/block-bindings/render.php#L336-L372

There was 1 failure:

1) WP_Block_Bindings_Render::test_default_binding_for_pattern_overrides
The `__default` attribute should be replaced with the real attribute prior to the callback.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<p>This is the content value</p>'
+'<p>This should not appear</p>'

/var/www/tests/phpunit/tests/block-bindings/render.php:358
/var/www/vendor/bin/phpunit:122

Thank you for the ping, @peterwilsoncc!
I've updated the PR to:

• fix the failed test;
• use the Reflection API to detect public properties, ensuring better compatibility with potential child classes (currently, none exist according to ​https://www.wpdirectory.net); context: ​https://github.com/WordPress/wordpress-develop/pull/6500#pullrequestreview-2329446989.

[joemcgill]

Just to add some more context to this ticket, given that we're getting close to the deadline for early tickets in the 6.8 milestone.

Even though, PHP 8.2 deprecated dynamic properties, they can still be supported by explicitly setting the AllowDynamicProperties attribute (​PHP docs). This was added to the WP_Block class in [54133].

Removing the need for that attribute is still worth pursuing, but we do have some runway before these result in errors. Given the number of classes that could potentially be affected, I'm curious why only WP_Block (this issue) and WP_Term (#61890) were singled out from #60875. @antonvlasenko or @hellofromTonya do you have any additional context to provide?

[audrasjb]

Moving this to 6.9 as we're getting close to 6.8 beta 1.

[antonvlasenko]

Replying to joemcgill:

Given the number of classes that could potentially be affected, I'm curious why only WP_Block (this issue) and WP_Term (#61890) were singled out from #60875. @antonvlasenko or @hellofromTonya do you have any additional context to provide?

Hi @joemcgill!
I think it's because, in my experience, there is no universal solution that applies to every class, although #60875 and #56034 provide guidelines.

Each class requires extensive research before implementing a solution that won’t break existing functionality or introduce new bugs. Additionally, a deep understanding of WordPress history is needed, as past decisions are not always well-documented or easy to interpret.

The lack of active reviewers who can provide timely feedback also plays a role, as very few people are interested in working on or reviewing such issues.

[SirLouen]

Test Report

Description

❌ This report can't validate that the indicated patch works as expected.

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/6500.diff

Environment

• WordPress: 6.9-alpha-60093-src
• PHP: 8.2.28
• Server: nginx/1.27.5
• Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
• Browser: Chrome 135.0.0.0
• OS: Windows 10/11
• Theme: Twenty Twenty-Five 1.2
• MU Plugins: None activated
• Plugins:
  • Test Reports 1.2.0
  • Test WP_Block Attributes 1.0

Patch Reproduction

• Looking at the patch, we can see the following WP-Block methods edited:

1. ::_get
2. ::__isset (new method)
3. ::__set (new method)
4. ::__unset (new method)

We could be looking at Blocks that actually use these 4 methods, but I've built a dummy block that procedurally executes these and can be downloaded from here: ​https://github.com/SirLouen/test-block-attributes/raw/refs/heads/main/test-block-attributes.zip

Expected Results

• Both, this method with attributes and dynamic attributes should work well
• The 3 deprecation notices should pop

Actual Results

1. ✅ Get, Set, Unset and Isset working for not dynamic attribs
2. ✅ Set, Unset and Isset working for dynamic attribs
3. ❌ Issues with Get for dynamic attribs

Tested in PHP 7.2, 8.2 and 8.4

Additional Notes

Here are my thoughts regarding this report:

Replying to antonvlasenko:

Each class requires extensive research before implementing a solution that won’t break existing functionality or introduce new bugs. Additionally, a deep understanding of WordPress history is needed, as past decisions are not always well-documented or easy to interpret.

Not necessarily. This is just anticipating too much, and AllowDynamicProperties avoid such forced deprecations with an unknown future yet.

It's true that there isn't a rule of thumb for sorting out all dynamic props, and probably we will see which classes require a revamp like this over the next couple of months/years. They could be sorted on the spot or preventively like this. It doesn't really matter. But the good thing is that each spotted case, its actually providing an use-case and this is good and necessary, while preventively sorting this kind of issues force the requirement of having to deploy a lot of code just to test these kinds of things.

From my point of view, it's worth simply testing under different scenarios and implementing as much as possible beautifully, carefully, and fleetly. But ideally, a way to test for testers, should come before implementation, not the other way around (and unit-tests are not enough).

Here's my best approach for testing this PR:

1. Review the code changes and assess whether the architecture is fine.
2. Ensure GitHub CI jobs pass.
3. Use the Site Editor. Test adding, updating and deleting blocks, ensuring there are no WP_Block related errors in the error log.

You can't say this! You are in the Test Team bruh!!!. This is why first we need to spot the code that already conflicting live, and then we should code to fix it, not the other way around (unless we are developing a new feat). I read this somewhere recently, but refactoring is very rare in WP unless there is a really good reason behind to prove everything.

Not abiding to this "non-written-rule" forces spending unnecessary time resources from many, not only the coder's time but others' time because rarely anyone will really want to step forward for something like this (unless you plan to commit by yourself and have someone that peer reviews it loosely, that also happens a lot, ngl).

I'm providing this test report as an example of what could be done preventively, imagine that its kind of exhausting if it should be done in each single ticket. I've made it just because I'm collecting a set of proofs (like this) to demonstrate that this behavior of coding before tests is problematic and leads to what I call needs-testing-workflow-waste (and the main root cause of +10-year-old tickets, dozens stuck in this situation)

Use-cases should be provided both in form of:

• A single existing (or not existing), plugin block, code, snippet, or a group of those, whatever, that we know that actually thoroughly tests each single aspect implemented here
• Unit-tests are never enough. Many times, Unit Tests are biased and just prove what we want to proof, and taking the time to review and look for potential biases in them is very exhausting and complicated. A real use-case it simpler and easy to test, edit and, more importantly, easier and less time-consuming to visualize for the average tester. Unit-tests are critical to future-proof any new development, and the best tool to drive a quality development. But not for external testers that come here new and have to review everything from scratch without a single pinch of knowledge of what's going on behind the scenes. And again, obviously just running the tests doesn't prove anything.

Supplemental Artifacts

• Full Plugin code here: ​https://github.com/SirLouen/test-block-attributes/