Skip to content
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

Fix script tag replacer #287

Merged
merged 4 commits into from
Aug 8, 2023
Merged

Fix script tag replacer #287

merged 4 commits into from
Aug 8, 2023

Conversation

puntope
Copy link
Contributor

@puntope puntope commented Aug 2, 2023

Changes proposed in this Pull Request:

Closes #279

Checks:

  • Does your code follow the WordPress coding standards?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Detailed test instructions:

  1. Dont check out this PR and create this filter
add_filter( 'script_loader_tag', array( $this, 'change_script_tags' ), 1, 3 );
function change_script_tags( $tag, $handle, $src ) {
		if ( in_array( $handle, self::ASYNC_SCRIPT_HANDLES, true ) ) {
			$tag = '<script type="text/javascript" src="' . $src . '" id="' . $handle . '-js"></script>';
		}
		return $tag;
	}
  1. Go to the Shop and search for google-tag-manager script in the source
  2. See "async" attribute not being added
  3. Checkout this PR
  4. Refresh
  5. See "async" attribute being added

Additional details:

Changelog entry

Fix - Add async attribute in google-tag-manager script.

@puntope puntope requested a review from a team August 2, 2023 13:33
@puntope puntope self-assigned this Aug 2, 2023
@github-actions github-actions bot added changelog: fix Took care of something that wasn't working. type: bug The issue/PR is a confirmed bug. labels Aug 2, 2023
eason9487
eason9487 previously approved these changes Aug 3, 2023
Copy link
Member

@eason9487 eason9487 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix.

This is not a blocker, but it would be a bit better to avoid having two async attributes, even though it won't cause any real problems. (One of the async attributes seems to come from WC-Blocks)

image

@puntope
Copy link
Contributor Author

puntope commented Aug 3, 2023

Howdy @eason9487 thanks for that catch. Might you test again with this change f571294 ?

Thanks

@puntope puntope requested a review from eason9487 August 3, 2023 10:24
@@ -722,6 +722,13 @@ public function async_script_loader_tags( $tag, $handle, $src ) {
return $tag;
}

return str_replace( '<script src', '<script async src', $tag );
// Check if the script has the async attribute already. If so, don't add it again.
$has_async_tag = preg_match( '/^.*async.*$/', $tag );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two async attributes still occur as this regex pattern cannot match correctly. Maybe consider matching it with a pattern like /\basync\b/?

@puntope
Copy link
Contributor Author

puntope commented Aug 7, 2023

Thanks @eason9487, for me it seems like working in Firefox and Chrome... but I replaced it with your solution here e3dd72c

Can you check one more time?
Thanks

@puntope puntope requested a review from eason9487 August 7, 2023 10:22
Copy link
Member

@eason9487 eason9487 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. LGTM.

for me it seems like working in Firefox and Chrome... [...]

Not sure what makes them different. In my testing env, the $tag content includes two script tags at a time and is separated by a linebreak char as the following, so the previous regex pattern couldn't match.

<script async src='https://www.googletagmanager.com/gtag/js?id=G-XXXXXXXXXX' id='google-tag-manager-js'></script>
<script id='google-tag-manager-js-after'>
	window.dataLayer = window.dataLayer || [];
	function gtag(){dataLayer.push(arguments);}
	gtag('js', new Date());
	gtag('config', 'G-XXXXXXXXXX', { 'send_page_view': false });
</script>

@puntope puntope merged commit 4696a3c into trunk Aug 8, 2023
3 checks passed
@puntope puntope deleted the fix/async-attribute-tags branch August 8, 2023 08:39
@jorgemd24 jorgemd24 mentioned this pull request Aug 8, 2023
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Took care of something that wasn't working. type: bug The issue/PR is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change to use wp_enqueue has made gtag.js load synchronously
2 participants