-
Notifications
You must be signed in to change notification settings - Fork 834
Forms: Fix feedback source data #45231
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
74c8f6f
572a2c6
64db6e2
0ab34a7
5909d7d
dec4b58
2a6ffa1
814e30a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
Significance: patch | ||
Type: fixed | ||
|
||
Forms: Store the feedback source info with more context | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,9 +17,9 @@ class Feedback_Source { | |
/** | ||
* The ID of the post or page that the feedback was created on. | ||
* | ||
* @var int | ||
* @var string | ||
enejb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
*/ | ||
private $id = 0; | ||
private $id = ''; | ||
|
||
/** | ||
* The title of the post or page that the feedback was created on. | ||
|
@@ -43,29 +43,62 @@ class Feedback_Source { | |
*/ | ||
private $page_number = 1; | ||
|
||
/** | ||
* The source type of the feedback entry. | ||
* Possible values: single, widget, block_template, block_template_part | ||
* | ||
* @var string | ||
*/ | ||
private $source_type = 'single'; | ||
|
||
/** | ||
* The request URL of the feedback entry. | ||
* | ||
* @var string | ||
*/ | ||
private $request_url = ''; | ||
|
||
/** | ||
* Constructor for Feedback_Source. | ||
* | ||
* @param int $id The ID of the feedback entry. | ||
* @param string $title The title of the feedback entry. | ||
* @param int $page_number The page number of the feedback entry, default is 1. | ||
* @param string|int $id The Source ID = post ID, widget ID, block template ID, or 0 for homepage or non-post/page. | ||
* @param string $title The title of the feedback entry. | ||
* @param int $page_number The page number of the feedback entry, default is 1. | ||
* @param string $source_type The source type of the feedback entry, default is 'single'. | ||
* @param string $request_url The request URL of the feedback entry. | ||
*/ | ||
public function __construct( $id, $title, $page_number = 1 ) { | ||
public function __construct( $id = 0, $title = '', $page_number = 1, $source_type = 'single', $request_url = '' ) { | ||
|
||
if ( is_numeric( $id ) ) { | ||
$this->id = $id > 0 ? $id : 0; | ||
} else { | ||
$this->id = $id; | ||
} | ||
|
||
$this->id = $id > 0 ? (int) $id : 0; | ||
$this->title = $title; | ||
$this->page_number = $page_number; | ||
$this->permalink = $this->id === 0 ? home_url() : ''; | ||
|
||
if ( $id <= 0 ) { | ||
return; | ||
} | ||
$this->permalink = empty( $request_url ) ? home_url() : $request_url; | ||
$this->source_type = $source_type; // possible source types: single, widget, block_template, block_template_part | ||
$this->request_url = $request_url; | ||
|
||
$entry_post = get_post( $id ); | ||
if ( is_numeric( $id ) && ! empty( $id ) ) { | ||
$entry_post = get_post( (int) $id ); | ||
if ( $entry_post && $entry_post->post_status === 'publish' ) { | ||
$this->permalink = get_permalink( $entry_post ); | ||
$this->title = get_the_title( $entry_post ); | ||
} elseif ( $entry_post ) { | ||
$this->permalink = ''; | ||
|
||
if ( $entry_post && $entry_post->post_status === 'publish' ) { | ||
$this->permalink = get_permalink( $entry_post ); | ||
$this->title = get_the_title( $entry_post ); | ||
if ( $entry_post->post_status === 'trash' ) { | ||
/* translators: %s is the post title */ | ||
$this->title = sprintf( __( '(trashed) %s', 'jetpack-forms' ), $this->title ); | ||
} | ||
} | ||
if ( empty( $entry_post ) ) { | ||
/* translators: %s is the post title */ | ||
$this->title = sprintf( __( '(deleted) %s', 'jetpack-forms' ), $this->title ); | ||
$this->permalink = ''; | ||
} | ||
} | ||
} | ||
|
||
|
@@ -83,11 +116,81 @@ public static function from_submission( $current_post, int $current_page_number | |
return new self( 0, '', $current_page_number ); | ||
} | ||
|
||
$title = $current_post->post_title ?? ''; | ||
$title = $current_post->post_title ?? __( '(no title)', 'jetpack-forms' ); | ||
|
||
return new self( $id, $title, $current_page_number ); | ||
} | ||
|
||
/** | ||
* Get the title of the current page. That we can then use to display in the feedback entry. | ||
* | ||
* @return string The title of the current page. That we want to show to the user. To tell them where the feedback was left. | ||
*/ | ||
private static function get_source_title() { | ||
if ( is_front_page() ) { | ||
return get_bloginfo( 'name' ); | ||
} | ||
if ( is_home() ) { | ||
return get_the_title( get_option( 'page_for_posts', true ) ); | ||
} | ||
if ( is_singular() ) { | ||
return get_the_title(); | ||
} | ||
if ( is_archive() ) { | ||
return get_the_archive_title(); | ||
} | ||
if ( is_search() ) { | ||
/* translators: %s is the search term */ | ||
return sprintf( __( 'Search results for: %s', 'jetpack-forms' ), get_search_query() ); | ||
} | ||
if ( is_404() ) { | ||
return __( '404 Not Found', 'jetpack-forms' ); | ||
} | ||
return get_bloginfo( 'name' ); | ||
} | ||
|
||
/** | ||
* Creates a Feedback_Source instance for a block template. | ||
* | ||
* @param array $attributes Form Shortcode attributes. | ||
* | ||
* @return Feedback_Source Returns an instance of Feedback_Source. | ||
*/ | ||
public static function get_current( $attributes ) { | ||
global $wp, $page; | ||
$current_url = home_url( add_query_arg( array(), $wp->request ) ); | ||
if ( isset( $attributes['widget'] ) && ! empty( $attributes['widget'] ) ) { | ||
return new self( $attributes['widget'], self::get_source_title(), 1, 'widget', $current_url ); | ||
} | ||
|
||
if ( isset( $attributes['block_template'] ) && ! empty( $attributes['block_template'] ) ) { | ||
global $_wp_current_template_id; | ||
return new self( $_wp_current_template_id, self::get_source_title(), $page, 'block_template', $current_url ); | ||
} | ||
|
||
if ( isset( $attributes['block_template_part'] ) && ! empty( $attributes['block_template_part'] ) ) { | ||
return new self( $attributes['block_template_part'], self::get_source_title(), $page, 'block_template_part', $current_url ); | ||
} | ||
|
||
return new Feedback_Source( \get_the_ID(), \get_the_title(), $page, 'single', $current_url ); | ||
} | ||
|
||
/** | ||
* Creates a Feedback_Source instance from serialized data. | ||
* | ||
* @param array $data The serialized data. | ||
* @return Feedback_Source Returns an instance of Feedback_Source. | ||
*/ | ||
public static function from_serialized( $data ) { | ||
$id = $data['source_id'] ?? 0; | ||
$title = $data['entry_title'] ?? ''; | ||
$page_number = $data['entry_page'] ?? 1; | ||
$source_type = $data['source_type'] ?? 'single'; | ||
$request_url = $data['request_url'] ?? ''; | ||
|
||
return new self( $id, $title, $page_number, $source_type, $request_url ); | ||
} | ||
|
||
/** | ||
* Get the permalink of the feedback entry. | ||
* | ||
|
@@ -100,6 +203,38 @@ public function get_permalink() { | |
return $this->permalink; | ||
} | ||
|
||
/** | ||
* Get the edit URL of the form or page where the feedback was submitted from. | ||
* | ||
* @return string The edit URL of the form or page. | ||
*/ | ||
public function get_edit_form_url() { | ||
|
||
if ( current_user_can( 'edit_theme_options' ) ) { | ||
if ( $this->source_type === 'block_template' && \wp_is_block_theme() ) { | ||
return admin_url( 'site-editor.php?p=' . esc_attr( '/wp_template/' . addslashes( $this->id ) ) . '&canvas=edit' ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I remember this template editor linking being particularly difficult because sometimes you need theme namespace added, or sometimes it's self created template. Worth testing well! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So far it has been working well for me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Jetpack supports current stable WP minus one, so no need to test anything older. I don't think there have been changes in recent versions to these URLs either. |
||
} | ||
|
||
if ( $this->source_type === 'block_template_part' && \wp_is_block_theme() ) { | ||
return admin_url( 'site-editor.php?p=' . esc_attr( '/wp_template_part/' . addslashes( $this->id ) ) . '&canvas=edit' ); | ||
} | ||
|
||
if ( $this->source_type === 'widget' && current_theme_supports( 'widgets' ) ) { | ||
return admin_url( 'widgets.php' ); | ||
} | ||
} | ||
|
||
if ( $this->id && is_numeric( $this->id ) && $this->id > 0 && current_user_can( 'edit_post', (int) $this->id ) ) { | ||
$entry_post = get_post( (int) $this->id ); | ||
if ( $entry_post && $entry_post->post_status === 'trash' ) { | ||
return ''; // No edit link is possible for trashed posts. They need to be restored first. | ||
} | ||
return \get_edit_post_link( (int) $this->id, 'url' ); | ||
} | ||
|
||
return ''; | ||
} | ||
|
||
/** | ||
* Get the relative permalink of the feedback entry. | ||
* | ||
|
@@ -131,7 +266,7 @@ public function get_title() { | |
/** | ||
* Get the post id of the feedback entry. | ||
* | ||
* @return int The ID of the feedback entry. | ||
* @return int|string The ID of the feedback entry. | ||
*/ | ||
public function get_id() { | ||
return $this->id; | ||
|
@@ -146,6 +281,9 @@ public function serialize() { | |
return array( | ||
'entry_title' => $this->title, | ||
'entry_page' => $this->page_number, | ||
'source_id' => $this->id, | ||
'source_type' => $this->source_type, | ||
'request_url' => $this->request_url, | ||
); | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(adding this as a comment on the changelog just to allow comment threading)
This is testing well! I did not test legacy theme or widget, but tested a range of scenarios with block themes.
404 page. I added a form to my 404 site editor template.
Form in footer template pattern. I added a form to my footer template. Then created a "Page with No Form".
Blog archive pagination: I set my home page to show blog posts, and then navigated to the third page of the blog archive (/page/3) and submitted the form that lives in the footer template pattern.
Page deletion. I have some other old form pages that I deleted. In the new PR, I now see (deleted) in the link, and the link just goes back to Jetpack Forms admin. That seems reasonable, but also wondering if we should just remove the link?
Tests pass. I also confirmed tests pass.
Only question: For deleted pages rather than link circularly back to forms admin, I wonder if we should just remove the link? Not a big deal though.
Verdict on merging: This touches sensitive logic and there are lot of possible permutations, so it's possible we'll find an issue that needs fixing. BUT there's a lot of clearly broken behavior now, and this fixes all of it so far. Given that, I'd say LGTM.