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

Improved page default status logic and add overwrite ability; ensure front page and page for posts can be served as AMP when enabled #872

Merged
merged 11 commits into from
Jan 18, 2018

Conversation

ThierryA
Copy link
Collaborator

This PR implements the workflow discussed here. For pages with custom templates assigned or for pages assigned to the front page or posts page, the default status will be disabled. For all others, the default status will be enabled.

The default status has a filter to allow devs to change the default status to disabled as such add_filter( 'amp_status_default_enabled', '__return_false' );

Closes #837
Closes #867

@ThierryA ThierryA added this to the v0.7 milestone Jan 17, 2018
@westonruter
Copy link
Member

@ThierryA do you think amp_status_default_enabled is better than amp_post_status_default_enabled since the status in question is tied to a given $post?

What would make most sense in light of theme support being added in 0.7? In 0.7 there is the available_callback which I see we're missing an important piece before calling. Namely, inside of \AMP_Theme_Support::is_paired_available() we need to add:

--- a/includes/class-amp-theme-support.php
+++ b/includes/class-amp-theme-support.php
@@ -77,6 +77,10 @@ class AMP_Theme_Support {
 			return false;
 		}
 
+		if ( is_singular() && ! post_supports_amp( get_queried_object() ) ) {
+			return false;
+		}
+
 		$args = array_shift( $support );
 
 		if ( isset( $args['available_callback'] ) && is_callable( $args['available_callback'] ) ) {

Naturally this shouldn't be part of this PR for 0.6.

I suppose what this comes down to is whether there would be a default status for non-post things, like terms. In that case, there could conceivably be a amp_term_status_default_enabled and a term_supports_amp() (however unlikely). Not a big deal either way for the filter naming, but maybe better to be explicit that it is specifically regarding posts to be more future proof.

@westonruter
Copy link
Member

As an extra safeguard, @ThierryA and I have discussed having the Page support not enabled by default. To enable Page support, the user would just enable Page checkbox in the AMP settings. By leaving it off by default, it will be safer to upgrade to 0.6.

@westonruter westonruter changed the title Improved page default status logic and add overwrite ability Improved page default status logic and add overwrite ability; ensure front page and page for posts can be served as AMP when enabled Jan 18, 2018
@westonruter
Copy link
Member

The three new commits here ensure that when you enable AMP for the homepage and posts (blog) page, that the AMP versions of them can indeed be served and that the templates can be filtered as required.

For example, if you want to serve out a custom page template for the homepage and posts page, you could put an amp/home.php and a amp/blog.php in your theme, and use them via the following in your functions.php:

function xyz_filter_home_and_blog_amp_post_template( $template, $template_type, $post ) {
	if ( 'page' === $template_type && 'page' === get_option( 'show_on_front' ) ) {
		if ( (int) get_option( 'page_on_front' ) === $post->ID ) {
			$template = dirname( __FILE__ ) . '/amp/home.php';
		} elseif ( (int) get_option( 'page_for_posts' ) === $post->ID ) {
			$template = dirname( __FILE__ ) . '/amp/blog.php';
		}
	}
	return $template;
}
add_filter( 'amp_post_template_file', 'xyz_filter_home_and_blog_amp_post_template', 10, 3 );

The posts page template would naturally need to have The Loop to actually show the posts in addition to the post content, and this could be accomplished by coping the single.php from the template and adding this after the .amp-wp-article:

	<ul>
		<?php while ( have_posts() ) : the_post(); ?>
			<li><a href="<?php the_permalink(); ?>"><?php the_title(); ?></a></li>
		<?php endwhile; ?>
	</ul>
	<?php the_posts_pagination(); ?>

/cc @mikeschinkel

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Just the one thing left to change page support to be opt-in via the admin screen.

@ThierryA
Copy link
Collaborator Author

ThierryA commented Jan 18, 2018

@ThierryA do you think amp_status_default_enabled is better than amp_post_status_default_enabled since the status in question is tied to a given $post?

@westonruter agreed, I pushed a commit which renames the filter to amp_post_status_default_enabled.

This PR should be good to go now.

@ThierryA
Copy link
Collaborator Author

Thanks for addressing the post meta sanitization issue @westonruter, approved!

@westonruter westonruter merged commit eeeaf5a into 0.6 Jan 18, 2018
@westonruter westonruter deleted the add/837 branch January 18, 2018 20:36
@mikeschinkel
Copy link
Contributor

@westonruter Are you assuming this means AMP support for a full site?

@westonruter
Copy link
Member

@mikeschinkel no, but this change here means that AMP is supported for all pages, including the homepage and blog page. AMP support for the entire site is being worked on in the context of amp theme support. Just wanted you to be aware of this update for 0.6.

@mikeschinkel
Copy link
Contributor

@westonruter Thanks. FYI, I ran into issues with the architecture and decided to work on a clean room implementation for my needs but with the potential of offering it back to this project if you are interested in it.

It is going slower than I'd like, but for supporting AMP I'm actually using the data in the protoascii files to drive conversion. The plan is to write a parser that converts protoascii into an object that I can var_dump() and then include that file in the plugin when needed, and my code then uses the rules in that file to decide how to generate code.

I think that's the best approach from a maintaining compatibility with future changes approach. Thoughts?

@westonruter
Copy link
Member

@mikeschinkel The plugin also is using data from the protoascii files. See https://github.com/Automattic/amp-wp/blob/develop/bin/amphtml-update.py

It generates class-amp-allowed-tags-generated.php which is then used for whitelist sanitization via AMP_Tag_And_Attribute_Sanitizer.

The sanitizer can then be called via:

$sanitized_content = AMP_Content_Sanitizer::sanitize( $content, amp_get_content_sanitizers(), $args )

Where amp_get_content_sanitizers() is a new helper function in 0.7-alpha.

@mikeschinkel
Copy link
Contributor

mikeschinkel commented Jan 18, 2018

@westonruter Interesting. Thanks. Will check it out as I work on my user case the next few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants