Opened 9 months ago
Last modified 2 weeks ago
#61154 reviewing defect (bug)
Fix the 'attributes' dynamic property in WP_Block
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.8 | Priority: | normal |
Severity: | normal | Version: | 6.6 |
Component: | Editor | Keywords: | has-patch has-unit-tests needs-testing php82 has-testing-info early |
Focuses: | php-compatibility | Cc: |
Description
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.
Change History (19)
This ticket was mentioned in PR #6500 on WordPress/wordpress-develop by @antonvlasenko.
9 months ago
#2
- Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed
Trac ticket: https://core.trac.wordpress.org/ticket/61154
#3
@
9 months ago
- Keywords needs-testing added
I've just submitted https://github.com/WordPress/wordpress-develop/pull/6500
This PR:
- Fixes the
WP_Block::$attributes
dynamic property by utilizing strategy proposed by @hellofromTonya in https://core.trac.wordpress.org/ticket/60875. Props @hellofromTonya. - Implements this suggestion.
- Adds unit tests to check how the newly declared magic methods on the
WP_Block
class handle theWP_Block::$attributes
property and other declared public properties.
#4
@
9 months ago
- Keywords php82 added
- Milestone changed from Awaiting Review to 6.6
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.
This ticket was mentioned in Slack in #core-php by antonvlasenko. View the logs.
9 months ago
#6
@
9 months ago
== Test Report
Please ignore, it was meant for https://core.trac.wordpress.org/ticket/60236
#8
@
8 months ago
- Milestone changed from 6.6 to 6.7
- Owner set to hellofromTonya
- Status changed from new to reviewing
#9
in reply to:
↑ 7
@
8 months ago
- Keywords has-testing-info added
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:
- Review the code changes and assess whether the architecture is fine.
- Ensure GitHub CI jobs pass.
- Use the Site Editor. Test adding, updating and deleting blocks, ensuring there are no
WP_Block
related errors in the error log.
#10
@
5 months ago
- Keywords early added
- Milestone changed from 6.7 to 6.8
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.
@antonvlasenko commented on PR #6500:
5 months ago
#11
@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.
@antonvlasenko commented on PR #6500:
5 months ago
#12
@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.
@peterwilsoncc commented on PR #6500:
4 weeks ago
#13
@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.
@peterwilsoncc commented on PR #6500:
4 weeks ago
#14
This change is causing the following test to fail as the __default
attribute is not replaced.
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
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
4 weeks ago
#16
@
4 weeks ago
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!
@antonvlasenko commented on PR #6500:
2 weeks ago
#18
This change is causing the following test to fail as the
__default
attribute is not replaced.
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.
I'm working on a patch for this ticket.