Skip to content

Fix preview of published post in reader mode#4663

Closed
ktmn wants to merge 1 commit intoampproject:developfrom
ktmn:develop
Closed

Fix preview of published post in reader mode#4663
ktmn wants to merge 1 commit intoampproject:developfrom
ktmn:develop

Conversation

@ktmn
Copy link
Copy Markdown

@ktmn ktmn commented May 6, 2020

Addresses #4656

@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no Has not signed the Google CLA label May 6, 2020
Copy link
Copy Markdown
Contributor

@pierlon pierlon left a comment

Choose a reason for hiding this comment

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

Great work here, @ktmn. Thanks for opening this PR!

A few lint errors to be resolved and we should be able to get this merged 😄.

case 'post':
return get_post( $this->ID );
$post = get_post( $this->ID );
if ( isset( $_GET['preview_id'] ) && isset( $_GET['preview_nonce'] ) ) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ( isset( $_GET['preview_id'] ) && isset( $_GET['preview_nonce'] ) ) {
if ( isset( $_GET['preview_id'], $_GET['preview_nonce'] ) ) {

$post = get_post( $this->ID );
if ( isset( $_GET['preview_id'] ) && isset( $_GET['preview_nonce'] ) ) {
$id = (int) $_GET['preview_id'];
if( wp_verify_nonce( $_GET['preview_nonce'], 'post_preview_' . $id ) ) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if( wp_verify_nonce( $_GET['preview_nonce'], 'post_preview_' . $id ) ) {
if ( wp_verify_nonce( $_GET['preview_nonce'], 'post_preview_' . $id ) ) {

@pierlon
Copy link
Copy Markdown
Contributor

pierlon commented May 6, 2020

@ktmn also, don't forget to sign the CLA, as outlined in #4663 (comment).

Copy link
Copy Markdown
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.

After digging into this, I found an alternative solution that I think will be more robust. All we need to do is pass in the initial $post when constructing the AMP_Post_Template and use it from then on. It will have the_preview applied already, so we don't need to call it manually:

--- a/includes/templates/class-amp-post-template.php
+++ b/includes/templates/class-amp-post-template.php
@@ -66,6 +66,13 @@ class AMP_Post_Template {
 	 */
 	public $ID;
 
+	/**
+	 * Post
+	 *
+	 * @var WP_Post
+	 */
+	public $post;
+
 	/**
 	 * AMP_Post_Template constructor.
 	 *
@@ -73,9 +80,11 @@ class AMP_Post_Template {
 	 */
 	public function __construct( $post ) {
 		if ( is_int( $post ) ) {
-			$this->ID = $post;
+			$this->ID   = $post;
+			$this->post = get_post( $post );
 		} elseif ( $post instanceof WP_Post ) {
-			$this->ID = $post->ID;
+			$this->ID   = $post->ID;
+			$this->post = $post;
 		}
 	}
 
@@ -151,22 +160,6 @@ class AMP_Post_Template {
 		$this->data = apply_filters( 'amp_post_template_data', $this->data, $this->post );
 	}
 
-	/**
-	 * Getter.
-	 *
-	 * @since 1.5
-	 *
-	 * @param string $name Property name.
-	 * @return mixed
-	 */
-	public function __get( $name ) {
-		switch ( $name ) {
-			case 'post':
-				return get_post( $this->ID );
-		}
-		return null;
-	}
-
 	/**
 	 * Get template directory for Reader mode.
 	 *
diff --git a/includes/templates/reader-template-loader.php b/includes/templates/reader-template-loader.php
index d518343dc..5833bc9ba 100644
--- a/includes/templates/reader-template-loader.php
+++ b/includes/templates/reader-template-loader.php
@@ -32,5 +32,5 @@ do_action( 'pre_amp_render_post', get_queried_object_id() );
 require_once AMP__DIR__ . '/includes/amp-post-template-functions.php';
 amp_post_template_init_hooks();
 
-$amp_post_template = new AMP_Post_Template( get_queried_object_id() );
+$amp_post_template = new AMP_Post_Template( $post );
 $amp_post_template->load();

@ktmn
Copy link
Copy Markdown
Author

ktmn commented May 7, 2020

@westonruter Yeah that makes sense, go with that.

@ktmn ktmn closed this May 7, 2020
@westonruter westonruter mentioned this pull request May 7, 2020
3 tasks
@westonruter
Copy link
Copy Markdown
Member

@ktmn cool. Please test the changes in #4665.

pierlon added a commit that referenced this pull request May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: no Has not signed the Google CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants