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

AMP Standard: Admin menu item no longer shows in older versions of WP #6793

Closed
aaemnnosttv opened this issue Dec 16, 2021 · 10 comments
Closed
Assignees
Labels
Bug Something isn't working P2 Low priority

Comments

@aaemnnosttv
Copy link
Contributor

Bug Description

The AMP admin menu item #wp-admin-bar-amp no longer appears in WP versions below 5.6 when using the Standard mode.

Expected Behaviour

I would expect the menu item to be present in WP 4.9+ as it was in previous releases
image

Screenshots

No response

PHP Version

7.4

Plugin Version

2.2.0

AMP plugin template mode

Standard

WordPress Version

4.9 < 5.6

Site Health

`

wp-core

version: 5.5
site_language: en_US
user_language: en_US
timezone: +00:00
permalink: /index.php/%year%/%monthnum%/%day%/%postname%/
https_status: true
user_registration: 0
blog_public: 1
default_comment_status: open
multisite: false
user_count: 1
dotorg_communication: true

wp-paths-sizes

wordpress_path: /home/zivowikilukebano/webapps/xikela
wordpress_size: 44.44 MB (46600588 bytes)
uploads_path: /home/zivowikilukebano/webapps/xikela/wp-content/uploads
uploads_size: 2.65 MB (2774215 bytes)
themes_path: /home/zivowikilukebano/webapps/xikela/wp-content/themes
themes_size: 9.37 MB (9820981 bytes)
plugins_path: /home/zivowikilukebano/webapps/xikela/wp-content/plugins
plugins_size: 22.40 MB (23485035 bytes)
database_size: 6.61 MB (6930432 bytes)
total_size: 85.46 MB (89611251 bytes)

wp-active-theme

name: Twenty Twenty (twentytwenty)
version: 1.8
author: the WordPress team
author_website: https://wordpress.org/
parent_theme: none
theme_features: core-block-patterns, automatic-feed-links, custom-background, post-thumbnails, custom-logo, title-tag, html5, align-wide, responsive-embeds, customize-selective-refresh-widgets, editor-color-palette, editor-font-sizes, editor-styles, amp, widgets, menus, editor-style
theme_path: /home/zivowikilukebano/webapps/xikela/wp-content/themes/twentytwenty
auto_update: Disabled

wp-themes-inactive (5)

Twenty Fifteen: version: 1.9, author: the WordPress team (latest version: 3.0),Auto-updates disabled
Twenty Nineteen: version: 2.1, author: the WordPress team,Auto-updates disabled
Twenty Seventeen: version: 1.4, author: the WordPress team (latest version: 2.8),Auto-updates disabled
Twenty Sixteen: version: 1.4, author: the WordPress team (latest version: 2.5),Auto-updates disabled
Twenty Twenty-One: version: 1.4, author: the WordPress team,Auto-updates disabled

wp-plugins-active (3)

AMP: version: 2.2.0, author: AMP Project Contributors, Auto-updates disabled
WP Downgrade | Specific Core Version: version: 1.2.2, author: Reisetiger, Auto-updates disabled
WP Rollback: version: 1.7.1, author: Impress.org, Auto-updates disabled

wp-plugins-inactive (5)

Akismet Anti-Spam: version: 4.0.1, author: Automattic (latest version: 4.2.1), Auto-updates disabled
Gutenberg: version: 4.9.0, author: Gutenberg Team, Auto-updates disabled
Site Kit by Google: version: 1.48.0, author: Google, Auto-updates disabled
Site Kit by Google Dev Settings: version: 0.4.0, author: Google, Auto-updates disabled
Site Kit by Google Tester: version: 1.6.0, author: Google, Auto-updates disabled

wp-media

image_editor: WP_Image_Editor_GD
imagick_module_version: Not available
imagemagick_version: Not available
file_uploads: File uploads is turned off
post_max_size: 256M
upload_max_filesize: 256M
max_effective_size: 256 MB
max_file_uploads: 20
gd_version: bundled (2.1.0 compatible)
ghostscript_version: unknown

wp-server

server_architecture: Linux 5.4.0-88-generic x86_64
httpd_software: Apache/2.4.51 (Unix) OpenSSL/1.1.1f
php_version: 7.4.26 64bit
php_sapi: fpm-fcgi
max_input_variables: 1000
time_limit: 30
memory_limit: 256M
max_input_time: 60
upload_max_size: 256M
php_post_max_size: 256M
curl_version: 7.68.0 OpenSSL/1.1.1f
suhosin: false
imagick_availability: false
pretty_permalinks: true

wp-database

extension: mysqli
server_version: 10.5.13-MariaDB-1:10.5.13+maria~focal
client_version: mysqlnd 7.4.26

wp-constants

WP_HOME: undefined
WP_SITEURL: undefined
WP_CONTENT_DIR: /home/zivowikilukebano/webapps/xikela/wp-content
WP_PLUGIN_DIR: /home/zivowikilukebano/webapps/xikela/wp-content/plugins
WP_MAX_MEMORY_LIMIT: 256M
WP_DEBUG: false
WP_DEBUG_DISPLAY: true
WP_DEBUG_LOG: false
SCRIPT_DEBUG: false
WP_CACHE: false
CONCATENATE_SCRIPTS: undefined
COMPRESS_SCRIPTS: undefined
COMPRESS_CSS: undefined
WP_LOCAL_DEV: undefined
DB_CHARSET: utf8
DB_COLLATE: undefined

wp-filesystem

wordpress: writable
wp-content: writable
uploads: writable
plugins: writable
themes: writable

amp_wp

amp_slug_query_var: amp
amp_slug_defined_late: false
amp_mode_enabled: standard
amp_reader_theme: legacy
amp_templates_enabled: post, page, is_singular, is_home, is_archive, is_author, is_date, is_search, is_404, is_category, is_tag
amp_serve_all_templates: true
amp_css_transient_caching_disabled: false
amp_css_transient_caching_threshold: 5000 transients per day
amp_css_transient_caching_sampling_range: 14 days
amp_css_transient_caching_transient_count: 41
amp_css_transient_caching_time_series:
20211216: 0
amp_libxml_version: 2.9.10

`

Gutenberg Version

No response

OS(s) Affected

No response

Browser(s) Affected

No response

Device(s) Affected

No response

Acceptance Criteria

No response

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

@aaemnnosttv aaemnnosttv added the Bug Something isn't working label Dec 16, 2021
@dhaval-parekh
Copy link
Collaborator

I am able to reproduce this bug. After debugging found that the below code segment prevent it from adding the AMP node in the admin bar.

// Note: This is somewhat of a shortcut for blocking access to the validation UI on unsupported versions of WP.
// It works because we're already using this method in many places to check if the user interface should be
// shown, and its the user interface which is particularly problematic on older versions of WP due to the
// JavaScript dependencies.
if ( ! $this->dependency_support->has_support() ) {
return false; // @codeCoverageIgnore
}

@milindmore22
Copy link
Collaborator

More discussion can be found under this comment

@westonruter
Copy link
Member

westonruter commented Dec 16, 2021

@aaemnnosttv Why is the site running WP 5.5? Certain features of the plugin are disabled in versions older than 5.6 as advised on the Settings screen.

That being said, whether the Settings link is displayed probably shouldn't be bound up with whether user has access to DevTools. It should be whether the user can manage_options.

@westonruter westonruter added the P2 Low priority label Dec 16, 2021
@westonruter westonruter modified the milestones: v2.3, v2.2.1 Dec 16, 2021
@aaemnnosttv
Copy link
Contributor Author

Why is the site running WP 5.5?

@westonruter this came up in our E2E tests on Site Kit where we actively test with AMP using WP 4.7 and 4.9 🙂 Is the AMP validation indicator in the admin bar no longer available for older versions of WP?

@westonruter
Copy link
Member

Is the AMP validation indicator in the admin bar no longer available for older versions of WP?

That's right. We generally now hide the UI for the validation screens in WP<5.6. So if on an old version of WP, the site will behave as if the user had turned off AMP DevTools. This is so we don't have to maintain support those screens on old versions of WordPress. It's not only the validation screens at issue here but it's also the AMP Validation sidebar in the block editor. Since we can't show the AMP Validation sidebar in WP<5.6 due to the necessary JS APIs not being present, it's also largely necessary to hide validation screens elsewhere. Ref #6775 (comment).

@westonruter
Copy link
Member

That being said, whether the Settings link is displayed probably shouldn't be bound up with whether user has access to DevTools. It should be whether the user can manage_options.

Oh, I misread the original description. I thought it was saying that the Settings submenu item alone was missing. But the issue is the entire AMP menu item is missing. If so, then this is working as expected given that we hide the AMP validation UI in WP<5.6. If in Standard mode, there's little value to have an AMP menu item since there is no link to the non-AMP version. So it would only contains the Settings link. And in that case, the settings screen is available on the admin menu already.

If we did still show an AMP menu item when DevTools is turned off, then the top-level menu item would have to be a non-link, as right now it's linking to Validate URL. Or else I suppose it could link to the Settings screen instead. But yeah, I don't see much value in having such a bare AMP menu item on every page. Only value I see is it showing that AMP is enabled for the page, but that can be just as well achieved by having the AMP Validator browser extension installed.

@aaemnnosttv
Copy link
Contributor Author

Only value I see is it showing that AMP is enabled for the page, but that can be just as well achieved by having the AMP Validator browser extension installed.

@westonruter we use the official AMP validator library for checking the page output as well, but only for non-logged in users. We use the AMP indicator specifically for checking the validity for a logged-in user since there are things there that it will ignore that the validator extension does not like data-ampdevmode (at least last time I checked!).

We generally now hide the UI for the validation screens in WP<5.6. So if on an old version of WP, the site will behave as if the user had turned off AMP DevTools.

We don't need to access the screen, but the validation state (✅ /⚠️) which was shown in the admin bar is what we were looking for. Is it not possible/useful to still show this even if the user isn't able to go the validation screen?

@dhaval-parekh
Copy link
Collaborator

I think we can show the AMP menu item in the admin bar and disable specific sub-menu items based on it.
This PR #6797 might address this issue.

@westonruter
Copy link
Member

We don't need to access the screen, but the validation state (✅ /⚠️) which was shown in the admin bar is what we were looking for. Is it not possible/useful to still show this even if the user isn't able to go the validation screen?

Sorry for the delay. Yes, this is something we've had filed for a while in #5166. It's not implemented yet, of course.

If you wanted to add your own plugin that sets a flag in the document to detect whether there were validation errors, even without being logged-in to have the admin bar being shown, you could taken an alternative approach like this: https://gist.github.com/westonruter/4ff8545e653efeeafe5b994c530a568e

You could use that, for example, to set an attribute on the html element to indicate there were validation errors detected and then look for that in your tests.

@westonruter
Copy link
Member

As such, I think I'll go ahead and close this.

@westonruter westonruter removed this from the v2.3 milestone Jan 25, 2022
@github-project-automation github-project-automation bot moved this to In Progress in Ongoing Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working P2 Low priority
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants