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

Add support for allowing a site subset to be native AMP #1235

Merged
merged 34 commits into from
Jul 2, 2018
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
143cd2a
WIP
westonruter Jun 22, 2018
6149da1
WIP2
westonruter Jun 24, 2018
fab22d4
WIP3
westonruter Jun 26, 2018
718e643
WIP4
westonruter Jun 27, 2018
1d2053c
WIP5
westonruter Jun 27, 2018
2eda94b
WIP6
westonruter Jun 28, 2018
3377ecf
WIP7
westonruter Jun 28, 2018
e5f4b77
Remove needless get_query_template
westonruter Jun 28, 2018
a7bf31e
WIP8
westonruter Jun 28, 2018
f440592
WIP9 [ci skip]
westonruter Jun 29, 2018
8f46ee7
WIP10 [ci skip]
westonruter Jun 29, 2018
0131c52
WIP11 [ci skip]
westonruter Jun 29, 2018
5288ccb
WIP12 [ci skip]
westonruter Jun 29, 2018
3f4ef4c
WIP13 [ci skip]
westonruter Jun 29, 2018
a616e84
Don't show post enable/disable if template is not available
westonruter Jun 30, 2018
a7371d1
Simplify logic in AMP_Post_Meta_Box::render_status
westonruter Jun 30, 2018
1adad3e
Fix tests regarding amp status metabox
westonruter Jun 30, 2018
fafb94e
Mock the gfycat requests during unit tests
westonruter Jun 30, 2018
95fd958
Let template_availability return supported when no templates match bu…
westonruter Jul 1, 2018
7e56c8b
Add unrecognized_templates_supported for Other templates
westonruter Jul 1, 2018
bfb202a
Let theme support args define templates_supported
westonruter Jul 1, 2018
2062816
Persist user selection even when checkbox disabled
westonruter Jul 1, 2018
1c759e5
Add tests for read_theme_support, is_support_added_via_option, get_th…
westonruter Jul 1, 2018
221c3f0
Improve testing of finish_init; add test stubs
westonruter Jul 2, 2018
3d07eac
Discard matching is_home when there are other matching templates
westonruter Jul 2, 2018
c32d76b
Elimiante Other/unrecognized template option since indistinguishable …
westonruter Jul 2, 2018
1bc4251
Restore the availability of AMP components in non-AMP documents in na…
westonruter Jul 2, 2018
11ea3d1
Remove unused variable and fix phpcs issue
westonruter Jul 2, 2018
4bde27d
Add tests for AMP_Theme_Support::get_supportable_templates()
westonruter Jul 2, 2018
c172763
Add test for AMP_Theme_Support::get_template_availability()
westonruter Jul 2, 2018
38b5546
Add test for custom template
westonruter Jul 2, 2018
acf9849
Restore available_callback but mark as deprecated
westonruter Jul 2, 2018
7091b35
Fix generation of AMP permalink for attachments
westonruter Jul 2, 2018
b54fcc0
Fix initialization of paried mode and Customizer, and fix support opt…
westonruter Jul 2, 2018
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
54 changes: 41 additions & 13 deletions amp.php
Original file line number Diff line number Diff line change
Expand Up @@ -272,28 +272,55 @@ function amp_correct_query_when_is_front_page( WP_Query $query ) {
}

/**
* Whether this is in 'canonical mode.'
* Whether this is in 'canonical mode'.
*
* Themes can register support for this with `add_theme_support( 'amp' )`.
* Then, this will change the plugin from 'paired mode,' and it won't use its own templates.
* Nor output frontend markup like the 'rel' link. If the theme registers support for AMP with:
* `add_theme_support( 'amp', array( 'template_dir' => 'my-amp-templates' ) )`
* it will retain 'paired mode.
* Themes can register support for this with `add_theme_support( 'amp' )`:
*
* @return boolean Whether this is in AMP 'canonical mode'.
* add_theme_support( 'amp' );
*
* This will serve templates in native AMP by default but the user would be able to change the template mode
* from native to paired in the admin. To force only native to be available, such as when you are using AMP components
* in your theme templates, do:
*
* add_theme_support( 'amp', array(
* 'mode' => 'native',
* ) );
*
* If you want to force AMP to always be served on a given template, use the amp_supportable_templates filter,
* for example to always serve the Category template in AMP:
*
* add_filter( 'amp_supportable_templates', function( $templates ) {
* $templates['is_category']['supported'] = true;
* return $template;
* } );
*
* Or if you want to prevent AMP from ever being served on the homepage:
*
* add_filter( 'amp_supportable_templates', function( $templates ) {
* $templates['is_front_page']['supported'] = false;
* return $template;
* } );
*
* @todo There needs to be a required flag which is the same as forcing all to be supported. This would be the same in the UI as the all_templates_supported flag. Might as well rename all_templates_supported to just required. If required is set to be false, or if any of the supported templates are immutable, then this option should not be available.
*
* @return boolean Whether this is in AMP 'canonical' mode, that is whether it is native and there is not separate AMP URL current URL.
*/
function amp_is_canonical() {
$support = get_theme_support( 'amp' );
if ( true === $support ) {
return true;
if ( ! current_theme_supports( 'amp' ) ) {
return false;
}

$mode = 'native';
$support = get_theme_support( 'amp' );
Copy link
Member

Choose a reason for hiding this comment

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

May there be a more differentiating naming for current_theme_supports vs. get_theme_support? Probably I am missing some of the context but the naming appear a little confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

The current_theme_supports() and get_theme_support() are WordPress core functions. They are somewhat confusing. The former returns whether support has been added (boolean), and applies a filter to let plugins control whether support is present. The get_theme_support() on the other hand does not apply a filter, but rather it returns the args that were passed in for the original add_theme_support() call.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Makes sense now.

if ( is_array( $support ) ) {
$args = array_shift( $support );
if ( empty( $args['template_dir'] ) ) {
return true;
if ( isset( $args['mode'] ) ) {
$mode = $args['mode'];
} elseif ( ! empty( $args['template_dir'] ) ) {
$mode = 'paired'; // If there is a template_dir, then paired mode is implied.
}
}
return false;
return 'native' === $mode;
}

/**
Expand Down Expand Up @@ -426,6 +453,7 @@ function _amp_bootstrap_customizer() {

/**
* Redirects the old AMP URL to the new AMP URL.
*
* If post slug is updated the amp page with old post slug will be redirected to the updated url.
*
* @since 0.5
Expand Down
28 changes: 25 additions & 3 deletions includes/admin/class-amp-post-meta-box.php
Original file line number Diff line number Diff line change
Expand Up @@ -170,16 +170,38 @@ public function render_status( $post ) {
is_post_type_viewable( $post->post_type )
&&
current_user_can( 'edit_post', $post->ID )
&&
! amp_is_canonical()
// @todo What if a site does not have non-AMP pages? Formerly this was indicated via amp_is_canonical(). Now we need a flag for whether AMP is exclusive to a theme, in which case all templates and post types must render AMP. In other words, all_templates_supported.
);

if ( true !== $verify ) {
return;
}

$errors = AMP_Post_Type_Support::get_support_errors( $post );
$status = post_supports_amp( $post ) ? self::ENABLED_STATUS : self::DISABLED_STATUS;
$status = empty( $errors ) ? self::ENABLED_STATUS : self::DISABLED_STATUS;
$errors = array_diff( $errors, array( 'post-status-disabled' ) ); // Subtract the status which the metabox will allow to be toggled.

// @todo If ! supported and immutable, don't show the toggle at all?
// Handle special case of the static front page or page for posts.
if ( current_theme_supports( 'amp' ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible having a comment for this if-then-else?

Copy link
Member Author

Choose a reason for hiding this comment

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

To explain why the condition is here?


$query = new WP_Query();
if ( 'page' === $post->post_type ) {
$query->set( 'page_id', $post->ID );
} else {
$query->set( 'p', $post->ID );
}
$query->queried_object = $post;
$query->queried_object_id = $post->ID;
$query->parse_query_vars();

$availability = AMP_Theme_Support::get_template_availability( $query );
if ( $availability instanceof WP_Error && 0 !== count( array_diff( $availability->get_error_codes(), $errors, array( 'post-status-disabled' ) ) ) ) {
$errors[] = 'template-unavailable';
$status = self::DISABLED_STATUS;
}
}

$labels = array(
'enabled' => __( 'Enabled', 'amp' ),
'disabled' => __( 'Disabled', 'amp' ),
Expand Down
92 changes: 37 additions & 55 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -239,80 +239,62 @@ function amp_add_amphtml_link() {
* @see AMP_Post_Type_Support::get_support_errors()
*
* @param WP_Post $post Post.
*
* @return bool Whether the post supports AMP.
*/
function post_supports_amp( $post ) {
if ( amp_is_canonical() ) {
return true;
}

$errors = AMP_Post_Type_Support::get_support_errors( $post );

// Return false if an error is found.
if ( ! empty( $errors ) ) {
return false;
}

switch ( get_post_meta( $post->ID, AMP_Post_Meta_Box::STATUS_POST_META_KEY, true ) ) {
case AMP_Post_Meta_Box::ENABLED_STATUS:
return true;

case AMP_Post_Meta_Box::DISABLED_STATUS:
return false;

// Disabled by default for custom page templates, page on front and page for posts.
default:
$enabled = (
! (bool) get_page_template_slug( $post )
&&
! (
'page' === $post->post_type
&&
'page' === get_option( 'show_on_front' )
&&
in_array( (int) $post->ID, array(
(int) get_option( 'page_on_front' ),
(int) get_option( 'page_for_posts' ),
), true )
)
);

/**
* Filters whether default AMP status should be enabled or not.
*
* @since 0.6
*
* @param string $status Status.
* @param WP_Post $post Post.
*/
return apply_filters( 'amp_post_status_default_enabled', $enabled, $post );
}
return 0 === count( AMP_Post_Type_Support::get_support_errors( $post ) );
}

/**
* Are we currently on an AMP URL?
* Determine whether the current response being served as AMP.
*
* @since 1.0 This function can be called before the `parse_query` action because the 'amp' query var is specifically and exclusively used when 'amp' theme support is added.
* This function cannot be called before the parse_query action because it needs to be able
* to determine the queried object is able to be served as AMP.
*
* @see is_header_video_active()
* @return bool Whether it is the AMP endpoint.
*/
function is_amp_endpoint() {
global $wp_query;
if ( is_admin() || is_feed() || ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ) {
return false;
}

// When 'amp' theme support is (or will be added) then these are the conditions that are key to be checked.
if ( amp_is_canonical() || isset( $_GET[ amp_get_slug() ] ) ) { // WPCS: CSRF OK.
return true;
$did_parse_query = did_action( 'parse_query' );

if ( ! $did_parse_query ) {
_doing_it_wrong( __FUNCTION__, sprintf( esc_html__( "is_amp_endpoint() was called before the 'parse_query' hook was called. This function will always return 'false' before the 'parse_query' hook is called.", 'amp' ) ), '0.4.2' );
}

// Condition for non-theme support when /amp/ endpoint is used.
if ( false !== get_query_var( amp_get_slug(), false ) ) {
return true;
$has_amp_query_var = (
isset( $_GET[ amp_get_slug() ] ) // WPCS: CSRF OK.
||
false !== get_query_var( amp_get_slug(), false )
);

// When there is no query var and AMP is not canonical/native, then this is definitely not an AMP endpoint.
if ( ! $has_amp_query_var && ! amp_is_canonical() ) {
return false;
}

return false;
if ( current_theme_supports( 'amp' ) ) {
$template_available = true === AMP_Theme_Support::get_template_availability();
return amp_is_canonical() ? $template_available : $has_amp_query_var;
} else {

// Check if the queried object supports AMP.
if ( is_singular() || ( $wp_query instanceof WP_Query && $wp_query->is_posts_page ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to check that $wp_query->is_posts_page

/**
* Post.
*
* @var WP_Post $queried_object
*/
$queried_object = get_queried_object();
return post_supports_amp( $queried_object );
}

return false; // Legacy templates only support posts via AMP_Post_Template.
}
}

/**
Expand Down
73 changes: 56 additions & 17 deletions includes/class-amp-post-type-support.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@ class AMP_Post_Type_Support {
/**
* Get post types that plugin supports out of the box (which cannot be disabled).
*
* @deprecated
* @return string[] Post types.
*/
public static function get_builtin_supported_post_types() {
_deprecated_function( __METHOD__, '1.0' );
return array_filter( array( 'post' ), 'post_type_exists' );
}

Expand All @@ -27,17 +29,12 @@ public static function get_builtin_supported_post_types() {
* @return string[] Post types eligible for AMP.
*/
public static function get_eligible_post_types() {
return array_merge(
self::get_builtin_supported_post_types(),
array( 'page' ),
array_values( get_post_types(
array(
'public' => true,
'_builtin' => false,
),
'names'
) )
);
return array_values( get_post_types(
array(
'public' => true,
),
'names'
) );
}

/**
Expand All @@ -49,10 +46,11 @@ public static function get_eligible_post_types() {
* @since 0.6
*/
public static function add_post_type_support() {
$post_types = array_merge(
self::get_builtin_supported_post_types(),
AMP_Options_Manager::get_option( 'supported_post_types', array() )
);
if ( current_theme_supports( 'amp' ) && AMP_Options_Manager::get_option( 'all_templates_supported' ) ) {
$post_types = self::get_eligible_post_types();
} else {
$post_types = AMP_Options_Manager::get_option( 'supported_post_types', array() );
}
foreach ( $post_types as $post_type ) {
add_post_type_support( $post_type, amp_get_slug() );
}
Expand All @@ -72,8 +70,7 @@ public static function get_support_errors( $post ) {
}
$errors = array();

// Because `add_rewrite_endpoint` doesn't let us target specific post_types.
if ( isset( $post->post_type ) && ! post_type_supports( $post->post_type, amp_get_slug() ) ) {
if ( ! post_type_supports( $post->post_type, amp_get_slug() ) ) {
$errors[] = 'post-type-support';
}

Expand All @@ -94,6 +91,48 @@ public static function get_support_errors( $post ) {
$errors[] = 'skip-post';
}

$status = get_post_meta( $post->ID, AMP_Post_Meta_Box::STATUS_POST_META_KEY, true );
if ( $status ) {
if ( AMP_Post_Meta_Box::DISABLED_STATUS === $status ) {
$errors[] = 'post-status-disabled';
}
} else {
/*
* Disabled by default for custom page templates, page on front and page for posts, unless 'amp' theme
* support is present (in which case AMP_Theme_Support::get_template_availability() determines availability).
*/
$enabled = (
current_theme_supports( 'amp' )
||
(
! (bool) get_page_template_slug( $post )
&&
! (
'page' === $post->post_type
&&
'page' === get_option( 'show_on_front' )
&&
in_array( (int) $post->ID, array(
(int) get_option( 'page_on_front' ),
(int) get_option( 'page_for_posts' ),
), true )
)
)
);

/**
* Filters whether default AMP status should be enabled or not.
*
* @since 0.6
*
* @param string $status Status.
* @param WP_Post $post Post.
*/
$enabled = apply_filters( 'amp_post_status_default_enabled', $enabled, $post );
if ( ! $enabled ) {
$errors[] = 'post-status-disabled';
}
}
return $errors;
}
}
Loading