[wp-trac] [WordPress Trac] #61154: Fix the 'attributes' dynamic property in WP_Block
WordPress Trac
noreply at wordpress.org
Fri Apr 25 22:49:52 UTC 2025
#61154: Fix the 'attributes' dynamic property in WP_Block
-------------------------------------------------+-------------------------
Reporter: antonvlasenko | Owner: audrasjb
Type: defect (bug) | Status: reviewing
Priority: normal | Milestone: 6.9
Component: Editor | Version: 6.6
Severity: normal | Resolution:
Keywords: has-patch has-unit-tests php82 has- | Focuses: php-
testing-info early | compatibility
-------------------------------------------------+-------------------------
Changes (by SirLouen):
* keywords: has-patch has-unit-tests needs-testing php82 has-testing-info
early => has-patch has-unit-tests php82 has-testing-info early
Comment:
== 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 [comment:9 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 "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/
--
Ticket URL: <https://core.trac.wordpress.org/ticket/61154#comment:23>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform
More information about the wp-trac
mailing list