WordPress.org
Get WordPress

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#60743 closed defect (bug) (fixed)

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

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

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.
2 years 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
2 years 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.
2 years ago

​@swissspidy commented on ​PR #6245:
2 years ago #4

cc @gziolo @luisherranz for reviews

​@gziolo commented on ​PR #6245:
2 years 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
2 years ago

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

Marking as commit candidate, pending review from @gziolo

#7 @swissspidy
2 years ago

  • Keywords dev-feedback added

#8 @swissspidy
2 years ago

  • Resolution → fixed
  • Status reviewing → 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
2 years ago

  • Keywords fixed-major added
  • Resolution fixed
  • Status closed → reopened

​@swissspidy commented on ​PR #6245:
2 years 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
2 years ago

  • Keywords dev-reviewed added; dev-feedback removed

#12 @gziolo
2 years 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
2 years ago

  • Resolution → fixed
  • Status reopened → 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.