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

Revamp notice appearing in AMP Enabled toggle, revamp how plugin determines whether AMP can be disabled #5010

Merged
merged 27 commits into from
Jul 14, 2020
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
5bcc30a
Default to paired when amp theme support added
westonruter Jul 9, 2020
f3ba560
Eliminate use of theme support flags and amp_supportable_templates fi…
westonruter Jul 9, 2020
6f037ab
Merge branch 'develop' of github.com:ampproject/amp-wp into update/th…
westonruter Jul 10, 2020
490fe5a
Eliminate use of post type support to control post type support
westonruter Jul 9, 2020
723a7bb
Remove test kept after merge conflict
westonruter Jul 10, 2020
a6596ba
Fix tests for changes to post type support
westonruter Jul 10, 2020
dc1d73a
Add tests for migrating supported_post_types
westonruter Jul 10, 2020
88d1c8b
Migrated supported_templates to be stored as associative array
westonruter Jul 10, 2020
14aeece
Remove redundant isset checks with alternative to appease PhpStan
westonruter Jul 10, 2020
1919ec1
Remove _doing_it_wrong() since templates_supported is supported only …
westonruter Jul 10, 2020
ab52015
Add missing semicolons from JS
westonruter Jul 10, 2020
d85ec63
Make sure toggling all themes supported does not destroy individual s…
westonruter Jul 11, 2020
668d0d9
Migrate theme support flags to options when upgrading
westonruter Jul 11, 2020
2a13776
Defer doing amp_plugin_update until end of after_setup_theme
westonruter Jul 11, 2020
75487f4
Remove obsolete checks for password-protected posts
westonruter Jul 11, 2020
270d604
Remove obsolete error codes for page-on-front and page-for-posts
westonruter Jul 11, 2020
e1f3871
Fix tests and finish changes for get_status_and_errors
westonruter Jul 11, 2020
7a0ab77
Revamp AMP Unavailable notice in post editor
westonruter Jul 11, 2020
3a326c3
Restore using non-associative arrays to store supported post types an…
westonruter Jul 11, 2020
8c51a9d
Remove obsolete assertion
westonruter Jul 11, 2020
3315937
Use static closures
westonruter Jul 11, 2020
a39c212
Remove duplicate test data
westonruter Jul 11, 2020
fefff34
Remove invalid todo
westonruter Jul 11, 2020
dac8963
Merge branch 'develop' of github.com:ampproject/amp-wp into update/th…
westonruter Jul 12, 2020
6e190c5
Merge branch 'develop' of github.com:ampproject/amp-wp into update/th…
westonruter Jul 13, 2020
7df6a05
Use end of init instead of end of after_setup_theme to do DB upgrade
westonruter Jul 13, 2020
e07656a
Improve styling of AMP Unavailable notice
westonruter Jul 14, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions assets/css/src/amp-block-editor.css
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,16 @@
.wp-core-ui .edit-post-header .editor-post-preview {
height: 34px;
}

.amp-unavailable-notice {
width: 100%;
westonruter marked this conversation as resolved.
Show resolved Hide resolved
}

.amp-unavailable-notice details > summary {
font-weight: bold;
user-select: none;
westonruter marked this conversation as resolved.
Show resolved Hide resolved
}

.amp-unavailable-notice details[open] > summary {
margin-bottom: 1em;
}
24 changes: 16 additions & 8 deletions assets/src/block-editor/plugins/amp-toggle.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,24 @@ function AMPToggle( { isEnabled, onChange } ) {
Boolean( errorMessages.length ) &&
(
<Notice
status="warning"
status="info"
isDismissible={ false }
className="amp-unavailable-notice"
>
{
errorMessages.map( ( message, index ) => (
<RawHTML key={ index }>
{ message }
</RawHTML>
) )
}
<details>
<summary>
{ __( 'AMP Unavailable', 'amp' ) }
</summary>
{
errorMessages.map( ( message, index ) => (
<p key={ index }>
<RawHTML>
{ message }
</RawHTML>
</p>
) )
}
</details>
</Notice>
)
}
Expand Down
53 changes: 26 additions & 27 deletions includes/admin/class-amp-post-meta-box.php
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,13 @@ public function enqueue_block_assets() {
return;
}

$status_and_errors = self::get_status_and_errors( $post );

// Skip proceeding if there are errors blocking AMP and the user can't do anything about it.
if ( ! empty( $status_and_errors['errors'] ) && ! current_user_can( 'manage_options' ) ) {
return;
}

wp_enqueue_style(
self::BLOCK_ASSET_HANDLE,
amp_get_asset_url( 'css/' . self::BLOCK_ASSET_HANDLE . '.css' ),
Expand All @@ -234,12 +241,9 @@ public function enqueue_block_assets() {
true
);

$status_and_errors = self::get_status_and_errors( get_post() );
$error_messages = $this->get_error_messages( $status_and_errors['status'], $status_and_errors['errors'] );

$data = [
'ampSlug' => amp_get_slug(),
'errorMessages' => $error_messages,
'errorMessages' => $this->get_error_messages( $status_and_errors['errors'] ),
'hasThemeSupport' => ! amp_is_legacy(),
'isStandardMode' => amp_is_canonical(),
];
Expand Down Expand Up @@ -284,11 +288,16 @@ public function render_status( $post ) {
}

$status_and_errors = self::get_status_and_errors( $post );
$status = $status_and_errors['status'];
$status = $status_and_errors['status']; // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable -- Used in amp-enabled-classic-editor-toggle.php.
$errors = $status_and_errors['errors'];

// Skip showing any error message if the user doesn't have the ability to do anything about it.
if ( ! empty( $errors ) && ! current_user_can( 'manage_options' ) ) {
return;
}

// phpcs:disable VariableAnalysis.CodeAnalysis.VariableAnalysis
$error_messages = $this->get_error_messages( $status, $errors );
$error_messages = $this->get_error_messages( $errors );

$labels = [
'enabled' => __( 'Enabled', 'amp' ),
Expand Down Expand Up @@ -322,9 +331,6 @@ public static function get_status_and_errors( $post ) {
$availability = AMP_Theme_Support::get_template_availability( $post );
$status = $availability['supported'] ? self::ENABLED_STATUS : self::DISABLED_STATUS;
$errors = array_diff( $availability['errors'], [ 'post-status-disabled' ] ); // Subtract the status which the metabox will allow to be toggled.
if ( true === $availability['immutable'] ) {
$errors[] = 'status_immutable';
}
} else {
$errors = AMP_Post_Type_Support::get_support_errors( $post );
$status = empty( $errors ) ? self::ENABLED_STATUS : self::DISABLED_STATUS;
Expand All @@ -338,40 +344,33 @@ public static function get_status_and_errors( $post ) {
* Gets the AMP enabled error message(s).
*
* @since 1.0
* @param string $status The AMP enabled status.
* @param array $errors The AMP enabled errors.
* @see AMP_Post_Type_Support::get_support_errors()
*
* @param string[] $errors The AMP enabled errors.
* @return array $error_messages The error messages, as an array of strings.
*/
public function get_error_messages( $status, $errors ) {
public function get_error_messages( $errors ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think AMP_Post_Meta_Box::get_status_and_errors should be refactored into 2 separate methods? The returned status is now only being used in 1 instance (AMP_Post_Meta_Box::get_amp_enabled_rest_field), and the set of errors are used twice elsewhere (AMP_Post_Meta_Box::enqueue_block_assets and AMP_Post_Meta_Box::render_status).

Copy link
Member Author

Choose a reason for hiding this comment

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

It's being used both in AMP_Post_Meta_Box::sanitize_status() and AMP_Post_Meta_Box::render_status(), in the latter case it appears to not be used only because the usage is in the include file at the end of the method.

Nevertheless, I'm not in love with get_status_and_errors.

$settings_screen_url = admin_url( 'admin.php?page=' . AMP_Options_Manager::OPTION_NAME );

$error_messages = [];
if ( in_array( 'status_immutable', $errors, true ) ) {
if ( self::ENABLED_STATUS === $status ) {
$error_messages[] = __( 'Your site does not allow AMP to be disabled.', 'amp' );
} else {
$error_messages[] = __( 'Your site does not allow AMP to be enabled.', 'amp' );
}
}
Comment on lines -347 to -353
Copy link
Member Author

Choose a reason for hiding this comment

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

Byebye!

if ( in_array( 'template_unsupported', $errors, true ) || in_array( 'no_matching_template', $errors, true ) ) {
$error_messages[] = sprintf(
/* translators: %s is a link to the AMP settings screen */
__( 'There are no <a href="%s">supported templates</a> to display this in AMP.', 'amp' ),
esc_url( admin_url( 'admin.php?page=' . AMP_Options_Manager::OPTION_NAME ) )
__( 'There are no <a href="%s" target="_blank">supported templates</a>.', 'amp' ),
esc_url( $settings_screen_url )
);
}
if ( in_array( 'password-protected', $errors, true ) ) {
$error_messages[] = __( 'AMP cannot be enabled on password protected posts.', 'amp' );
}
if ( in_array( 'post-type-support', $errors, true ) ) {
$error_messages[] = sprintf(
/* translators: %s is a link to the AMP settings screen */
__( 'AMP cannot be enabled because this <a href="%s">post type does not support it</a>.', 'amp' ),
esc_url( admin_url( 'admin.php?page=' . AMP_Options_Manager::OPTION_NAME ) )
__( 'This post type is not <a href="%s" target="_blank">enabled</a>.', 'amp' ),
esc_url( $settings_screen_url )
);
}
if ( in_array( 'skip-post', $errors, true ) ) {
$error_messages[] = __( 'A plugin or theme has disabled AMP support.', 'amp' );
}
if ( count( array_diff( $errors, [ 'status_immutable', 'page-on-front', 'page-for-posts', 'password-protected', 'post-type-support', 'skip-post', 'template_unsupported', 'no_matching_template' ] ) ) > 0 ) {
if ( count( array_diff( $errors, [ 'post-type-support', 'skip-post', 'template_unsupported', 'no_matching_template' ] ) ) > 0 ) {
$error_messages[] = __( 'Unavailable for an unknown reason.', 'amp' );
}

Expand Down
5 changes: 2 additions & 3 deletions includes/admin/functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ function amp_admin_get_preview_permalink() {
$post_type = (string) apply_filters( 'amp_customizer_post_type', 'post' );

// Make sure the desired post type is actually supported, and if so, prefer it.
$supported_post_types = get_post_types_by_support( AMP_Post_Type_Support::SLUG );
$supported_post_types = AMP_Post_Type_Support::get_supported_post_types();
if ( in_array( $post_type, $supported_post_types, true ) ) {
$supported_post_types = array_unique( array_merge( [ $post_type ], $supported_post_types ) );
}
Expand All @@ -63,11 +63,10 @@ function amp_admin_get_preview_permalink() {
'no_found_rows' => true,
'suppress_filters' => false,
'post_status' => 'publish',
'post_password' => '',
'post_type' => $supported_post_types,
'posts_per_page' => 1,
'fields' => 'ids',
// @todo This should eventually do a meta_query to make sure there are none that have AMP_Post_Meta_Box::STATUS_POST_META_KEY = DISABLED_STATUS.
// @todo This should eventually do a meta_query to make sure there are none that have AMP_Post_Meta_Box::STATUS_POST_META_KEY = DISABLED_STATUS.
]
);

Expand Down
24 changes: 15 additions & 9 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,6 @@ function amp_init() {
add_action( 'wp_loaded', 'amp_bootstrap_admin' );

add_rewrite_endpoint( amp_get_slug(), EP_PERMALINK );
AMP_Post_Type_Support::add_post_type_support();
add_action( 'init', [ 'AMP_Post_Type_Support', 'add_post_type_support' ], 1000 ); // After post types have been defined.
add_action( 'parse_query', 'amp_correct_query_when_is_front_page' );
add_action( 'admin_bar_menu', 'amp_add_admin_bar_view_link', 100 );

Expand Down Expand Up @@ -143,14 +141,22 @@ function () {
*/
$options = get_option( AMP_Options_Manager::OPTION_NAME, [] );
$old_version = isset( $options[ Option::VERSION ] ) ? $options[ Option::VERSION ] : '0.0';

if ( AMP__VERSION !== $old_version && is_admin() && current_user_can( 'manage_options' ) ) {
/**
* Triggers when after amp_init when the plugin version has updated.
*
* @param string $old_version Old version.
*/
do_action( 'amp_plugin_update', $old_version );
AMP_Options_Manager::update_option( Option::VERSION, AMP__VERSION );
// This waits to happen until the very end of init to ensure that amp theme support and amp post type support have all been added.
add_action(
'init',
static function () use ( $old_version ) {
/**
* Triggers when after amp_init when the plugin version has updated.
*
* @param string $old_version Old version.
*/
do_action( 'amp_plugin_update', $old_version );
AMP_Options_Manager::update_option( Option::VERSION, AMP__VERSION );
},
PHP_INT_MAX
);
}

add_action(
Expand Down
26 changes: 18 additions & 8 deletions includes/class-amp-post-type-support.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public static function get_eligible_post_types() {
*/
public static function get_post_types_for_rest_api() {
return array_intersect(
get_post_types_by_support( 'amp' ),
self::get_supported_post_types(),
get_post_types(
[
'show_in_rest' => true,
Expand All @@ -67,21 +67,31 @@ public static function get_post_types_for_rest_api() {
);
}

/**
* Get supported post types.
*
* @return string[] List of post types that support AMP.
*/
public static function get_supported_post_types() {
if ( ! amp_is_legacy() && AMP_Options_Manager::get_option( Option::ALL_TEMPLATES_SUPPORTED ) ) {
return self::get_eligible_post_types();
}
return AMP_Options_Manager::get_option( Option::SUPPORTED_POST_TYPES, [] );
}

/**
* Declare support for post types.
*
* This function should only be invoked through the 'after_setup_theme' action to
* allow plugins/theme to overwrite the post types support.
*
* @codeCoverageIgnore
* @since 0.6
* @deprecated The 'amp' post type support is no longer used at runtime to determine whether AMP is supported.
*/
public static function add_post_type_support() {
if ( ! amp_is_legacy() && AMP_Options_Manager::get_option( Option::ALL_TEMPLATES_SUPPORTED ) ) {
$post_types = self::get_eligible_post_types();
} else {
$post_types = AMP_Options_Manager::get_option( Option::SUPPORTED_POST_TYPES, [] );
}
foreach ( $post_types as $post_type ) {
_deprecated_function( __METHOD__, '1.6' );
foreach ( self::get_supported_post_types() as $post_type ) {
add_post_type_support( $post_type, self::SLUG );
}
}
Expand All @@ -100,7 +110,7 @@ public static function get_support_errors( $post ) {
}
$errors = [];

if ( ! post_type_supports( $post->post_type, self::SLUG ) ) {
if ( ! in_array( $post->post_type, self::get_supported_post_types(), true ) ) {
$errors[] = 'post-type-support';
}

Expand Down
Loading