Standard AMP mode is being determined as secondary rather than primary AMP mode (V2 AMP plugin).

[techanvil]

Bug Description

When using the V2 AMP plugin and selecting Standard mode, it's being interpreted as 'secondary' mode in Site Kit, when it should be 'primary'.

This is because STANDARD_MODE_SLUG is being grouped with TRANSITIONAL_MODE_SLUG and READER_MODE_SLUG in this condition:

site-kit-wp/includes/Context.php

Lines 369 to 381 in dce817b

	if (
	in_array(
	$mode,
	array(
	AMP_Theme_Support::STANDARD_MODE_SLUG,
	AMP_Theme_Support::TRANSITIONAL_MODE_SLUG,
	AMP_Theme_Support::READER_MODE_SLUG,
	),
	true
	)
	) {
	return self::AMP_MODE_SECONDARY;
	}

Steps to reproduce

1. With the V2 AMP plugin installed, go to AMP Settings and select Standard mode.
2. Navigate to Site Kit.
3. Using the JS console, invoke the getAMPMode and isPrimaryAMP selectors. They will return 'secondary' and false respectively.

Screenshots

Additional Context

• PHP Version:
• OS: [e.g. iOS]
• Browser: [e.g. chrome, safari]
• Plugin Version: [e.g. 22]
• Device: [e.g. iPhone6]

---

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

Acceptance criteria

• The AMP mode returned by Context::get_amp_mode() (and related selectors) should always reflect the current AMP mode based on the AMP plugin configuration (if active) and whether the Web Stories plugin is active.
  • Concretely, this first step is effectively to revert Update to always return AMP mode secondary. #3577 (change the code to what it was before).
  • Additionally, for the logic paths where Context::get_amp_mode() returns false, add an extra condition for whether the Web Stories plugin is active (via defined( 'WEBSTORIES_VERSION' )), and if so return AMP_MODE_SECONDARY instead of false.
    • This is only needed for the false logic paths, since otherwise the site is already using AMP in some way, and since Web Stories are AMP, it would not affect the outcome in those cases.
• The module setup for Tag Manager should always require setting up AMP + Web containers (as today) if AMP is detected (i.e. Context::is_amp()) regardless of whether the AMP mode is primary or secondary.

Implementation Brief

• Revert changes made in Update to always return AMP mode secondary. #3577.
• In includes/Context.php:
  • Update the get_amp_mode() method:
    • Move the logic that returns AMP_MODE_SECONDARY when WEBSTORIES_VERSION is defined to only run when the remaining logic in the method returns false, i.e. if any other logic in the method returns false, check if WEBSTORIES_VERSION is defined and return AMP_MODE_SECONDARY in that case.
• In assets/js/modules/tagmanager/components/common/WebContainerSelect.js:
  • Remove the logic that returns null when isPrimaryAMP is true.

Test Coverage

• In assets/js/modules/tagmanager/components/common/WebContainerSelect.test.js:
  • Remove the test case labelled should render nothing in a primary AMP context.
• Fix any other failing test(s).

QA Brief

• The Tag Manager module setup should show both Web and AMP container if any amp mode is active, including web stories plugin.
• googlesitekit.data.select('core/site').getAMPMode() should return correct amp mode (primary/secondary).

Changelog entry

• Ensure the determined AMP mode correctly reflects the AMP plugin configuration (if active) and whether the Web Stories plugin is active.

[aaemnnosttv]

@felixarntz looking back I found #2998 where we intentionally changed AMP primary to be always handled as secondary which essentially makes it impossible for isPrimaryAMP to ever return true. Was the idea that this might be restored at some point or should we simplify our AMP handling to remove logic around AMP primary entirely?

[felixarntz]

@techanvil The ACs overall look good, I added a bit more detail under the first point, to clarify that this is effectively a revert (plus one single change that had also been found in the related issue #2998).

If this looks good to you, feel free to move to IB.

[techanvil]

Thanks @felixarntz, the updated AC LGTM, moving to IB 👍

[eugene-manuilov]

• Revert changes made in Feature/3521 idea hub remove local setting #3557.

@nfmohit we need to revert #3577 instead of #3557

Additionally, for the logic paths where Context::get_amp_mode() returns false, add an extra condition for whether the Web Stories plugin is active (via defined( 'WEBSTORIES_VERSION' )), and if so return AMP_MODE_SECONDARY instead of false.

• This is only needed for the false logic paths, since otherwise the site is already using AMP in some way, and since Web Stories are AMP, it would not affect the outcome in those cases.

This is missing in IB

[nfmohit]

Thank you for your kind review on this, @eugene-manuilov!

@nfmohit we need to revert #3577 instead of #3557

Whoops, that was a typo. I've updated it. Thank you!

Additionally, for the logic paths where Context::get_amp_mode() returns false, add an extra condition for whether the Web Stories plugin is active (via defined( 'WEBSTORIES_VERSION' )), and if so return AMP_MODE_SECONDARY instead of false.

• This is only needed for the false logic paths, since otherwise the site is already using AMP in some way, and since Web Stories are AMP, it would not affect the outcome in those cases.

This is missing in IB

I think I actually misunderstood this part earlier. I didn't add anything about this in the IB because the Context::get_amp_mode() method already checks for the Web Stories plugin at the very first and returns AMP_MODE_SECONDARY early.

site-kit-wp/includes/Context.php

Lines 325 to 330 in caa4384

	public function get_amp_mode() {
	// If the Web Stories plugin is enabled, consider the site to be running
	// in Secondary AMP mode.
	if ( defined( 'WEBSTORIES_VERSION' ) ) {
	return self::AMP_MODE_SECONDARY;
	}

However, now that I read the ACs again, I understand that this logic should only run if the remaining logic returns false. I have updated the IB.

Thank you!

[eugene-manuilov]

Thanks, @nfmohit. IB ✔️

[wpdarren]

QA Update: ✅

Verified:

• When setting up and editing Tag Manager, I can see the AMP container field.
• When Standard template mode is set within the AMP plugin, I can see that on the latest release, secondary is set when running the code in the QAB. When I switched to the develop branch, this is changed to primary.
• Set Transitional and Reader, and when the code is run, this is set to secondary.
• Tested with Web Stories enabled and disabled.

Screenshots