-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Reuse wp_script_attributes filter for adding data-wp-router-options attribute to script module
#72449
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
Conversation
…ttribute to script module
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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 guess this file can actually be deleted entirely now?
…pdate/script-module-router-options-attributes
|
Flaky tests detected in e519c02. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18610136860
|
…pdate/script-module-router-options-attributes
|
Thanks, Weston. @samueljseay and I have been looking at this. We still need to iron out some details, but we believe that this shows the changes we'd need in Gutenberg to maintain backward compatibility with versions prior to WordPress 6.9. If you agree, we can take over and ask you for a review once it's ready. EDIT: We will also take care of opening the PR in WordPress Core. |
| } | ||
| if ( $is_fully_interactive || ( $is_supports_interactive && $is_supports_client_navigation ) ) { | ||
| foreach ( $block_type->view_script_module_ids as $script_module_id ) { | ||
| gutenberg_interactive_script_modules_registry( $script_module_id ); |
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.
So this is adding the script to the static array inside of gutenberg_interactive_script_modules_registry(). That works, although it seems a bit unconventional.
| * Access the shared static variable for interactive script modules. | ||
| * | ||
| * @param string|null $script_module_id The script module ID to register, or null to get the list. | ||
| * @return array<string, true> Associative array of script module ID => true. |
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.
Might want to make this abundantly clear that this "script module ID" is the ID for the SCRIPT[type=module] tag, not the registered "Script Module". Both are being used in this function, which may be confusing.
|
|
||
| // Gets private methods via reflection. | ||
| $get_marked_for_enqueue = $reflection->getMethod( 'get_marked_for_enqueue' ); | ||
| $get_marked_for_enqueue->setAccessible( true ); |
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.
Aside: this is causing a PHP 8.5 deprecation warning:
Deprecated: Method ReflectionMethod::setAccessible() is deprecated since 8.5, as it has no effect in /var/www/src/wp-content/plugins/gutenberg/lib/compat/wordpress-6.9/script-modules.php on line 53
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.
Isn't that file removed in this PR?
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.
Yes, but I'm just saying this adds to the importance of getting this merged.
|
Just noting that I've migrated @westonruter 's changes from this branch in a new PR here: #72489 This can be closed. |
|
Closing as superseded by #72489. Thanks Weston! |
This is a follow-up to #72340 which removes the new introduction of the new
wp_script_module_attributesfilter in favor of reusing the existingwp_script_attributesfilter. Discussed in WordPress/wordpress-develop#10324 (comment)See #70873