-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Support unique IDs in iAPI directives processing #10284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Support unique IDs in iAPI directives processing #10284
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just reviewed this draft, and everything looks great. 😄
Some things could be polished; please check the comments below.
Ah, once you have the Trac ticket created, don't forget to update the ticket number in the test functions!
src/wp-includes/interactivity-api/class-wp-interactivity-api.php
Outdated
Show resolved
Hide resolved
$context = end( $this->context_stack ); | ||
private function evaluate( $entry ) { | ||
$context = end( $this->context_stack ); | ||
['namespace' => $ns, 'value' => $path] = $entry; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Just confirmed that associative array destructuring is supported since PHP 7.1, and WordPress 6.9 won't support PHP versions older than 7.2. 😄
src/wp-includes/interactivity-api/class-wp-interactivity-api.php
Outdated
Show resolved
Hide resolved
src/wp-includes/interactivity-api/class-wp-interactivity-api.php
Outdated
Show resolved
Hide resolved
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
I've addressed the requested changes, and I added the ticket. Let me know if I should change something else. |
Co-authored-by: David Arenas <[email protected]>
if ( ! $ns || ! $path ) { | ||
/* translators: %s: The directive value referenced. */ | ||
$message = sprintf( __( 'Namespace or reference path cannot be empty. Directive value referenced: %s' ), $directive_value ); | ||
$message = sprintf( __( 'Namespace or reference path cannot be empty. Directive value referenced: %s' ), json_encode( $entry ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it again, I feel we can get rid of this _doing_it_wrong
message entirely. It doesn't seem to add value to developers. Maybe we can move it to the get_directive_entries
function. 🤔
This PR adds the server-side logic to support unique IDs in the Interactivity API directives. The server should match what the client is doing. See: WordPress/gutenberg#72161. This way, the correct HTML will come directly from the server, and it will avoid layout shifts.
Additionally, it includes a small refactoring to how the directives are processed to follow similar patterns to the client code, and it includes a bunch of tests to ensure everything works as expected.
Trac ticket: https://core.trac.wordpress.org/ticket/64106
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.