WordPress.org
Get WordPress

Make WordPress Core

Opened 6 months ago

Closed 6 months ago

#60743 closed defect (bug) (fixed)

Interactivity API - SSR won't work if ant render_block_data filter edits $parsed_block

Reported by: cbravobernal's profile cbravobernal Owned by: swissspidy's profile swissspidy
Milestone: 6.5 Priority: normal
Severity: normal Version:
Component: Editor Keywords: has-patch commit fixed-major dev-reviewed
Focuses: Cc:

Description (last modified by cbravobernal)

As commented in ​https://github.com/WordPress/gutenberg/pull/59057#discussion_r1518417589

If someone edits the $parsed_block variable, it will fail within the Interactivity API SSR processing comparison:
​https://github.com/WordPress/wordpress-develop/blob/9a616a573434b432a5efd4de21039250658371fe/src/wp-includes/interactivity-api/interactivity-api.php#L49

It seems that this comparison won't be needed in 6.6, as a md5 bug did not allow to add keys to that variable.

Co-authored with @santosguillamot

Change History (13)

This ticket was mentioned in ​PR #6245 on ​WordPress/wordpress-develop by ​@cbravobernal.
6 months ago #1

  • Keywords has-patch has-unit-tests added

Trac ticket:
https://core.trac.wordpress.org/ticket/60743#ticket

As commented in ​https://github.com/WordPress/gutenberg/pull/59057#discussion_r1518417589
If someone edits the $parsed_block variable, it will fail within the Interactivity API SSR processing comparison:
​https://github.com/WordPress/wordpress-develop/blob/9a616a573434b432a5efd4de21039250658371fe/src/wp-includes/interactivity-api/interactivity-api.php#L49

It seems that this comparison won't be needed in 6.6, as a md5 bug did not allow to add keys to that variable.

#2 @cbravobernal
6 months ago

  • Description modified (diff)
  • Keywords has-patch has-unit-tests removed

This ticket was mentioned in ​Slack in #core-performance by cbravobernal. ​View the logs.
6 months ago

​@swissspidy commented on ​PR #6245:
6 months ago #4

cc @gziolo @luisherranz for reviews

​@gziolo commented on ​PR #6245:
6 months ago #5

I don't have enough insight to tell whether the priority 20, 99, or PHP_INT_MAX would work better here. The final approach should most likely impact the render_block_* filter used in this function as it uses exactly the same reasoning:

​https://github.com/WordPress/wordpress-develop/blob/3c2a87e2aeb71d20f72b4f94b15e1caf803e8f21/src/wp-includes/interactivity-api/interactivity-api.php#L60-L63

One important aspect to keep in mind is that we have been talking about when HTML API is capable of processing all HTML tags, we could explore running all the processing in a single pass based on the HTML output generated for the <body> tag in the document. In that case, we wouldn't need to use these filters. Anyway, we are still not there and we would also have to confirm that the performance is equal or improved before switching to that approach.

#6 @swissspidy
6 months ago

  • Component changed from General to Editor
  • Keywords has-patch commit added
  • Owner set to swissspidy
  • Status changed from new to reviewing

Marking as commit candidate, pending review from @gziolo

#7 @swissspidy
6 months ago

  • Keywords dev-feedback added

#8 @swissspidy
6 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 57826:

Interactivity API: Increase hook priority for processing directives.

Use a priority of 100 to ensure that other filters can add additional directives before the processing starts.
This way, directives will be processed even if the $parsed_block variable is edited by a filter.

Props cbravobernal, swissspidy, flixos90, joemcgill, gziolo.
Fixes #60743.

#9 @swissspidy
6 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

​@swissspidy commented on ​PR #6245:
6 months ago #10

Committed to trunk in https://core.trac.wordpress.org/changeset/57826

Needs double sign-off (dev-reviewed) for backporting to the 6.5 branch.

#11 @gziolo
6 months ago

  • Keywords dev-reviewed added; dev-feedback removed

#12 @gziolo
6 months ago

By the way, the changes were synced back to the Gutenberg plugin, which resolved some issues with e2e tests. That confirms that this fix is important.

#13 @swissspidy
6 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 57830:

Interactivity API: Increase hook priority for processing directives.

Use a priority of 100 to ensure that other filters can add additional directives before the processing starts.
This way, directives will be processed even if the $parsed_block variable is edited by a filter.

Reviewed by gziolo.
Merges [57826] to the to the 6.5 branch.

Props cbravobernal, swissspidy, flixos90, joemcgill, gziolo.
Fixes #60743.

Note: See TracTickets for help on using tickets.