Site-kit-wp: Detect existing snippets also in secondary AMP version

[felixarntz]

Bug Description

We currently detect existing Analytics/AdSense snippets by looking at the homepage. This presents a problem for sites using secondary AMP versions (which is not preferred, but still very common).

We should ensure that, if the site is using AMP in a mode that relies on a secondary version, we trigger a second fetch request to one of these URLs too.

See #413 for the original report that lead to this issue, and #83 for related JS detection improvements.

This issue relies on #83 being completed.

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

• In order to solve this issue in a way that will also help with other issues (such as #413), there should be a general detection mechanism for AMP usage, based on the AMP plugin.
• If the AMP plugin is used in "Transitional" or "Reader" mode, the getExistingTag() function should fire a secondary request to an AMP URL. Since "Reader" mode will only have AMP content available for single post URLs, this secondary AMP request should generally be sent to the URL of a single post (e.g. the first one found).
• Related: If no tag is found, the result should not be undefined because that is the value used by the cache as essentially "no value cached".

Implementation Brief

• Add the following methods to the Context class in PHP:
  
  
  
  • is_amp(): Should be the same as AMP_Trait::is_amp(). All code using this trait should use that new method instead, and the trait should be removed.
  
  
  • get_amp_mode(): Should check if the function is_amp_endpoint() from the AMP plugin exists (to see if it's active), and then retrieve the AMP plugin's option for template mode. The method should return primary (if "Standard" mode), secondary (if "Transitional" or "Reader" mode), or just false (if AMP plugin is not being used). This also allows this method to be used as if ( get_amp_mode() ) to see if AMP is being used at all.
  
  
• Expose the value of get_amp_mode() to JS in Assets::get_inline_data() via googlesitekit.admin.ampMode.
• In JS in the getExistingTag() method, if no existing tag was found by the existing homepage check, check if googlesitekit.admin.ampMode is "secondary", and if so:
  
  
  
  • issue a regular WP REST API query to wp/v2/posts to query a random single post
  
  
  • issue a fetch() call to the post's URL with the ?amp query parameter present, and then run the same detection logic (findTagInHtmlContent()) on the response markup
  
  

Changelog entry

• Detect existing Analytics and AdSense tags reliably also when using a secondary AMP version.

[aaemnnosttv]

IB LGTM! Just a few questions.

Expose the value of get_amp_mode() to JS in Assets::get_inline_data() via googlesitekit.admin.ampMode

Minor detail: there is a bit of an inconsistency with the existing AMP-related property on the same object. We should use the same naming convention for AMP for both. I prefer normal camelcase as you have used in this IB. Should we also update AMPenabled to ampEnabled ?

https://github.com/google/site-kit-wp/blob/7d4aac9711a1174dc76acfad601c3fcd50a0baca/includes/Core/Assets/Assets.php#L473
https://github.com/google/site-kit-wp/search?q=AMPenabled&unscoped_q=AMPenabled

if no existing tag was found by the existing homepage check, check if googlesitekit.admin.ampMode is "secondary", and if so... issue a regular WP REST API query to wp/v2/posts to query a random single post

I'm assuming you don't mean actually random, and that we should just check the first one (random as in which one doesn't really matter). Correct?
E.g. https://make.wordpress.org/core/wp-json/wp/v2/posts?limit=1

[felixarntz]

@aaemnnosttv

Should we also update AMPenabled to ampEnabled?

Makes sense, thanks for bringing this up!

I'm assuming you don't mean actually random, and that we should just check the first one (random as in which one doesn't really matter). Correct?

Exactly.

[aaemnnosttv]

IB approved ✅

[tofumatt]

I found this one a bit confusing to QA as I wasn't sure exactly what the STR the issue were, but here were my steps:

1. Install the AMP Plugin (https://en-gb.wordpress.org/plugins/amp/)
2. Check Analytics at http://googlekitlocal.10uplabs.com/wp-admin/admin.php?page=googlesitekit-settings
3. Notice the error message about an existing Analytics snippet being present in the code. (Note: there is no specific error; it simply says another plugin is inserting it. 🤷‍♂)
4. Disable the AMP plugin
5. Return to http://googlekitlocal.10uplabs.com/wp-admin/admin.php?page=googlesitekit-settings
6. See "Snippet is inserted" message with no existing snippet found

Curiously I don't need to insert a GA tracking code into http://googlekitlocal.10uplabs.com/wp-admin/admin.php?page=amp-analytics-options to get the error to disable, but maybe that's a quirk of the AMP plugin.

At any rate, this seems to detect an AMP version's GA code, so QA looks good.

[felixarntz]

@aaemnnosttv Found a minor issue during QA/approval: We don't add the same tagverify and cache busting arguments to the AMP requests, which may cause a tag by Site Kit itself to be detected. Can you fix this in a follow-up PR associated with this issue?

[aaemnnosttv]

Sure thing @felixarntz 👍

[felixarntz]

All looking good now! ✅