Skip to content

Add AMP on protected posts#3697

Merged
westonruter merged 4 commits intodevelopfrom
enhancement/3428-add-amp-on-protected-posts
Nov 8, 2019
Merged

Add AMP on protected posts#3697
westonruter merged 4 commits intodevelopfrom
enhancement/3428-add-amp-on-protected-posts

Conversation

@pierlon
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
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
Copy Markdown
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
Copy Markdown
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