Incompatibility with Polylang's language taxonomy

[swissspidy]

I installed Polylang on a site where I try to add AMP support in the theme. This mostly means a bunch of is_amp_endpoint() checks.

I've set up two languages, but haven't really done anything with them yet.

Now when I navigate to /page/2/?amp, is_amp_endpoint() ultimately calls \AMP_Theme_Support::get_template_availability which in turn calls \AMP_Theme_Support::get_supportable_templates().

Also in the list of supportable templates is the language taxonomy added by Polylang. This is because the AMP plugin only checks for publicly_queryable, not public. The taxonomy is queryable though, just not public.

This leads to a PHP notice in this part of the code:

amp-wp/includes/class-amp-theme-support.php

Lines 533 to 544 in 19f1f24

	// Delete all matching parent templates since the child will override them.
	if ( ! $has_children ) {
	$supportable_template = $supportable_templates[ $id ];
	while ( ! empty( $supportable_template['parent'] ) ) {
	$parent = $supportable_template['parent'];
	$supportable_template = $supportable_templates[ $parent ];
	// Let the child supported status override the parent's supported status.
	unset( $matching_templates[ $parent ] );
	}
	}
	}

[westonruter]

Did this result in is_amp_endpoint() always returning false, so that in transitional mode.you can never access an AMP page?

[westonruter]

Possibly-related forum topic: https://wordpress.org/support/topic/referenced-amp-url-is-not-an-amp-8/#post-11816408

[swissspidy]

It's been a while; I would have to test it again. But if I recall correctly, I was still able to access the AMP pages. There simply was a notice.

[jamesozzie]

AMP pages work fine but only when slug ID or name is visible (this box is unticked).

Another user with the same issue:
https://wordpress.org/support/topic/static-home-page-with-polylang-2/#post-11867170

[westonruter]

@jamesozzie I've added this to the current project board to prioritize fixing this for the next release.

[schlessera]

It seems that the check for 'publicly_queryable' is actually the wrong one, and it would need to be changed to public instead. After all, if it is not 'public', there is not frontend side to it, so there's also no point in checking for AMP validation...?

[westonruter]

But doesn't publicly_queryable inherit from public? I don't understand why it makes a difference. Nevertheless, this change does prevent the notice:

diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php
index e02cf242..d620f355 100644
--- a/includes/class-amp-theme-support.php
+++ b/includes/class-amp-theme-support.php
@@ -892,8 +892,8 @@ class AMP_Theme_Support {
 		}
 
 		$taxonomy_args = [
-			'_builtin'           => false,
-			'publicly_queryable' => true,
+			'_builtin' => false,
+			'public'   => true,
 		];
 		foreach ( get_taxonomies( $taxonomy_args, 'objects' ) as $taxonomy ) {
 			$templates[ sprintf( 'is_tax[%s]', $taxonomy->name ) ] = [

However, perhaps it is only fixing the symptom and not the root cause?

I'm getting this specific notice:

PHP Notice: Undefined index: is_archive in /app/public/content/plugins/amp/includes/class-amp-theme-support.php on line 689

Here:

amp-wp/includes/class-amp-theme-support.php

Line 689 in 75674df

$supportable_template = $supportable_templates[ $parent ];

I get this when I go to a URL like https://wordpressdev.lndo.site/es/?amp where the language code is the endpoint.

For my environment, $supportable_templates is:

{
    "is_singular": {
        "label": "Singular",
        "description": "Required for the above content types.",
        "user_supported": true,
        "immutable": false,
        "supported": true
    },
    "is_front_page": {
        "label": "Homepage",
        "parent": "is_singular",
        "user_supported": false,
        "immutable": false,
        "supported": true
    },
    "is_home": {
        "label": "Blog",
        "user_supported": false,
        "immutable": false,
        "supported": true
    },
    "is_archive": {
        "label": "Archives",
        "user_supported": false,
        "immutable": false,
        "supported": true
    },
    "is_author": {
        "label": "Author",
        "parent": "is_archive",
        "user_supported": false,
        "immutable": false,
        "supported": true
    },
    "is_date": {
        "label": "Date",
        "parent": "is_archive",
        "user_supported": false,
        "immutable": false,
        "supported": true
    },
    "is_search": {
        "label": "Search",
        "user_supported": false,
        "immutable": false,
        "supported": true
    },
    "is_404": {
        "label": "Not Found (404)",
        "user_supported": false,
        "immutable": false,
        "supported": true
    },
    "is_category": {
        "label": "Categor\u00edas",
        "parent": "is_archive",
        "user_supported": false,
        "immutable": false,
        "supported": true
    },
    "is_tag": {
        "label": "Etiquetas",
        "parent": "is_archive",
        "user_supported": false,
        "immutable": false,
        "supported": true
    },
    "is_tax[language]": {
        "label": "Languages",
        "parent": "is_archive",
        "callback": {},
        "user_supported": false,
        "immutable": false,
        "supported": true
    }
}

The issue seems to be that is_tax[language] has a parent template condition of is_archive but the supportable template for is_archive is getting unset here:

amp-wp/includes/class-amp-theme-support.php

Lines 667 to 671 in 75674df

	// Make sure children override their parents.
	$matching_template_ids = array_keys( $matching_templates );
	foreach ( array_diff( array_keys( $supportable_templates ), $matching_template_ids ) as $template_id ) {
	unset( $supportable_templates[ $template_id ] );
	}

Apparently the logic in #1235 is not properly accounting for the template hierarchy, perhaps here due to the fact that is_tax[language] has a parent of is_archive instead of is_tax?

A related issue that may serve as inspiration here is #1938.

[schlessera]

I currently fail to replicate this.

I tried to verify the box mentioned in #1786 (comment) , but I don't have that one in my version. What version of the Polylang plugin are you running? I'm trying to replicate with version 2.6.4...

[jamesozzie]

@schlessera If you set your homepage to a static page that option should appear.

[schlessera]

@jamesozzie Yes, that does indeed display the missing check box.

However, no matter what settings I use, I'm still unable to trigger a notice. I'm monitoring the PHP error log, and I tried trigger an E_USER_NOTICE myself, which I could see appear in the log.

Maybe I'm misunderstanding the ticket and looking in the wrong place, so I'll recount what I tried to get a notice:

• I installed and activate Polylang (via WP-CLI).
• I created a new child theme with genesis as a parent theme.
• I set the AMP mode to Transitional.
• I created 2 languages within Polylang.
• I created a post that was translated.
• I created a page that was translated.
• I set the page to be the static front page.
• I added a single-php template and used the is_amp_endpoint() in there.
• I added var_dump()and error_log() statements to make sure the template with the is_amp_endpoint() is being used.
• I tried accessing all of the above content with and without the ?amp suffix.

[schlessera]

@swissspidy Are you able to replicate this? Do you have any pointers for me in that regard?

[schlessera]

Could finally replicate the notice. This is the one I'm geeting:

[23-Oct-2019 08:26:59 UTC] PHP Notice:  Undefined index: is_archive in /Users/alain/Sites/amp-wp/wp-content/plugins/amp/includes/class-amp-theme-support.php on line 719
[23-Oct-2019 08:26:59 UTC] PHP Stack trace:
[23-Oct-2019 08:26:59 UTC] PHP   1. {main}() /Users/alain/.composer/vendor/weprovide/valet-plus/server.php:0
[23-Oct-2019 08:26:59 UTC] PHP   2. require() /Users/alain/.composer/vendor/weprovide/valet-plus/server.php:131
[23-Oct-2019 08:26:59 UTC] PHP   3. require() /Users/alain/Sites/amp-wp/index.php:17
[23-Oct-2019 08:26:59 UTC] PHP   4. require_once() /Users/alain/Sites/amp-wp/wp-blog-header.php:19
[23-Oct-2019 08:26:59 UTC] PHP   5. include() /Users/alain/Sites/amp-wp/wp-includes/template-loader.php:78
[23-Oct-2019 08:26:59 UTC] PHP   6. get_footer() /Users/alain/Sites/amp-wp/wp-content/themes/twentytwenty/index.php:117
[23-Oct-2019 08:26:59 UTC] PHP   7. locate_template() /Users/alain/Sites/amp-wp/wp-includes/general-template.php:76
[23-Oct-2019 08:26:59 UTC] PHP   8. load_template() /Users/alain/Sites/amp-wp/wp-includes/template.php:671
[23-Oct-2019 08:26:59 UTC] PHP   9. require_once() /Users/alain/Sites/amp-wp/wp-includes/template.php:722
[23-Oct-2019 08:26:59 UTC] PHP  10. wp_footer() /Users/alain/Sites/amp-wp/wp-content/themes/twentytwenty/footer.php:58
[23-Oct-2019 08:26:59 UTC] PHP  11. do_action() /Users/alain/Sites/amp-wp/wp-includes/general-template.php:2761
[23-Oct-2019 08:26:59 UTC] PHP  12. WP_Hook->do_action() /Users/alain/Sites/amp-wp/wp-includes/plugin.php:465
[23-Oct-2019 08:26:59 UTC] PHP  13. WP_Hook->apply_filters() /Users/alain/Sites/amp-wp/wp-includes/class-wp-hook.php:310
[23-Oct-2019 08:26:59 UTC] PHP  14. wp_admin_bar_render() /Users/alain/Sites/amp-wp/wp-includes/class-wp-hook.php:286
[23-Oct-2019 08:26:59 UTC] PHP  15. do_action_ref_array() /Users/alain/Sites/amp-wp/wp-includes/admin-bar.php:86
[23-Oct-2019 08:26:59 UTC] PHP  16. WP_Hook->do_action() /Users/alain/Sites/amp-wp/wp-includes/plugin.php:531
[23-Oct-2019 08:26:59 UTC] PHP  17. WP_Hook->apply_filters() /Users/alain/Sites/amp-wp/wp-includes/class-wp-hook.php:310
[23-Oct-2019 08:26:59 UTC] PHP  18. AMP_Validation_Manager::add_admin_bar_menu_items() /Users/alain/Sites/amp-wp/wp-includes/class-wp-hook.php:286
[23-Oct-2019 08:26:59 UTC] PHP  19. is_amp_endpoint() /Users/alain/Sites/amp-wp/wp-content/plugins/amp/includes/validation/class-amp-validation-manager.php:497
[23-Oct-2019 08:26:59 UTC] PHP  20. AMP_Theme_Support::get_template_availability() /Users/alain/Sites/amp-wp/wp-content/plugins/amp/includes/amp-helper-functions.php:338

[westonruter]

Excellent. Same as I was getting in #1786 (comment).

[schlessera]

I think the root cause of the notice is that for is_tax[language], the array of $matching_templates ends up being only is_tax[language], where it should actually include is_archive as well.

The reason why the is_archive is not included is that the is_tax[language] is not public, therefore has no archive page of its own.

As the supported_templates gets stripped of everything that is not part of matching_templates, this also strips is_archive, so it cannot be retrieved anymore as the parent of $is_tax[language].

Following is an overview of the three different ways I think this might be solved, and what that entails.

1.) Harden the logic that removes the parent templates to gracefully the case where a parent template is not found in the $supported_templates array. This gets rid of the PHP notice, but lets the logic continue with is_tax[language] as the template to be checked for support. However, this seems to be conceptually wrong ni this instance, as the page to be rendered is the home page or a static front page, not a taxonomy archive.

2.) Change the query arguments from publicly_queryable = true to public = true. This will avoid adding is_tax[language] to the array of $supported_templates, as it fails this condition. Therefore, we'll end up with an empty $matching_templates array and the check will depend on all_templates_supported instead. This seems to be the correct behavior, as an entity that is not public does not have a front-facing representation (even though it might still be publicly_queryable).

3.) Do both the changes in 1.) & 2.) but adapt 1.) to throw a _doing_it_wrong() instead of a notice. The main problem we're encountering is that we have a template that matches while its parent doesn't. That shouldn't happen in a proper hierarchy. Therefore, throwing a _doing_it_wrong() like we do a bit further down (https://github.com/ampproject/amp-wp/blob/1.3/includes/class-amp-theme-support.php#L794-L804) makes the site owner aware of this. But we first need to ensure we actually query for the right taxonomies and post types in the first place, hence the change in 2.).

4.) Engage with the Polylang developers to discuss the rewrite rules that the plugin uses, as the URL that causes this issue is not actually a taxonomy archive page.

I would suggest going with option 3.) as the conceptually most correct change. We can still do 4.) in parallel, but we can immediately solve the problem on our end at least with 3.).

@westonruter, @swissspidy Thoughts?

[westonruter]

@schlessera Excellent research and suggestions. Going with 3 seems good to me.

What is the impact going to be in the admin screen with that change? Is it the case that the Polylang taxonomy currently is being listed among the checkboxes, but with the change it will now be removed?

Thinking about it, it doesn't seem to make sense for a user to ever land on archive template with the queried object being the Polylang language taxonomy. Is that even the behavior of Polylang presently? If I have English and Spanish on my site, then it doesn't make sense for me to access Spanish, as rather Polylang should be routing the queried object to the translated entity rather than the language term itself. If not, then it indeed doesn't make sense for the taxonomy to be listed among the supportable templates.

[schlessera]

What is the impact going to be in the admin screen with that change? Is it the case that the Polylang taxonomy currently is being listed among the checkboxes, but with the change it will now be removed?

Yes, it would be removed.

And I agree, it doesn't make sense to land on that template, I think it is an issue with the rewrite rules from Polylang not catching this properly.

I'll go ahead with implementing 3.) then.

[schlessera]

Testing Instructions:

1. Install and activate the Polylang plugin.
2. Go to the AMP Settings page.
3. Disable the option "Serve all templates as AMP regardless of what is being queried".
4. Verify: Under "Templates", the "Archives" > "Languages" template should not appear.

[csossi]

Hi @schlessera - FE is fine - ok for me to move to Ready To Merge? Any logs you need to check first?