Make WordPress Core

Opened 9 months ago

Last modified 2 weeks ago

#61154 reviewing defect (bug)

Fix the 'attributes' dynamic property in WP_Block

Reported by: antonvlasenko's profile antonvlasenko Owned by: audrasjb's profile audrasjb
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)

#1 @antonvlasenko
9 months ago

I'm working on a patch for this ticket.

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

#3 @antonvlasenko
9 months ago

  • Keywords needs-testing added

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.
Last edited 9 months ago by antonvlasenko (previous) (diff)

#4 @ironprogrammer
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 @faisal03
9 months ago

== Test Report

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

Last edited 9 months ago by faisal03 (previous) (diff)

#7 follow-up: @hmbashar
8 months ago

How can I test the issue?

#8 @hellofromTonya
8 months ago

  • Milestone changed from 6.6 to 6.7
  • Owner set to hellofromTonya
  • Status changed from new to reviewing

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.

#9 in reply to: ↑ 7 @antonvlasenko
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:

  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.

#10 @hellofromTonya
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.

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

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


4 weeks ago

#16 @audrasjb
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!

#17 @audrasjb
3 weeks ago

  • Owner changed from hellofromTonya to audrasjb

Self assigning for review.

@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.

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:

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 weeks ago

Note: See TracTickets for help on using tickets.