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 AMP on protected posts #3697

Merged
merged 4 commits into from
Nov 8, 2019

Conversation

pierlon
Copy link
Contributor

@pierlon pierlon commented Nov 7, 2019

Summary

  • Password protected post no longer disables AMP
  • Password form is now shown for Reader mode

Fixes #3428.

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@googlebot googlebot added the cla: yes Signed the Google CLA label Nov 7, 2019
$errors[] = 'password-protected';
// Show password form instead of the post content.
if ( ! current_theme_supports( 'amp' ) && post_password_required( $post ) ) {
add_filter( 'the_content', [ __CLASS__, 'show_password_form' ] );
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this isn't really the right place for this filter to be added. Adding a filter is introducing global side effects which don't seem necessarily expected.

Copy link
Member

Choose a reason for hiding this comment

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

I think this can be simplified quite a bit: 8519e4b

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that is indeed a much better solution. Thanks 👌

@westonruter westonruter added this to the v1.5 milestone Nov 8, 2019
@@ -322,7 +322,7 @@ private function build_post_comments_data() {
*/
private function build_post_content() {
$amp_content = new AMP_Content(
$this->post->post_content,
post_password_required( $this->post ) ? get_the_password_form( $this->post ) : $this->post->post_content,
Copy link
Member

Choose a reason for hiding this comment

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

This is now somewhat like what get_the_content() is doing:

	// If post password required and it doesn't match the cookie.
	if ( post_password_required( $_post ) ) {
		return get_the_password_form( $_post );
	}

@pierlon
Copy link
Contributor Author

pierlon commented Nov 8, 2019

Thanks for simplifying the password form logic, @westonruter.

@westonruter westonruter merged commit 4a45440 into develop Nov 8, 2019
@westonruter westonruter deleted the enhancement/3428-add-amp-on-protected-posts branch November 8, 2019 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for forcing AMP on protected posts
3 participants