-
Notifications
You must be signed in to change notification settings - Fork 329
Output consent mode JS via wp_print_inline_script_tag()
#9726
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
felixarntz
left a comment
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.
@westonruter This looks good to me, but it would be great to get someone more active in the project to approve as a 2nd sign-off.
@aaemnnosttv Would you be able to get this into one of the next releases? It blocks WordPress/performance#1686 that we're exploring to improve performance implications of gtag.
(I believe Site Kit may have a few more spots where it would ideally use wp_print_inline_script_tag() rather than direct output of the script. Maybe that's something to do in a separate PR, if you agree.)
aaemnnosttv
left a comment
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.
Thanks @westonruter – this looks like a nice improvement, although like I mentioned on the issue, our minimum supported version of WP is 5.2 but this function wasn't added until 5.7. We do have a BC helper class which makes this available, but without it, this is breaking E2E in the older version.
https://github.com/google/site-kit-wp/actions/runs/11945338420/job/33297846941?pr=9726#step:11:387
I also noted another instance where this could be applied which may not have been there when you opened this, I'm not sure. LMK if you have any questions 👍
|
Note: I do not have Site Kit set up for local development, so these changes I applied via the GitHub UI and thus I'm relying on GHA to check for any problems. |
|
The E2E tests are passing for WordPress 5.2.21, AMP 1.5.5. But they're failing for latest and trunk. The errors don't seem to indicate they're related to the code in this PR, however, so perhaps they just need to be restarted. |
aaemnnosttv
left a comment
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.
Great, thanks @westonruter !
Summary
Addresses issue:
wp_print_inline_script_tag()#9725Relevant technical choices
Use
wp_print_inline_script_tag()andwp_print_script_tag()rather than manually print script tags.PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist