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.

Sign up for GitHub

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

Refactor for the fix_post_row_actions function #442

Closed
wants to merge 4 commits into from
Closed

Refactor for the fix_post_row_actions function #442

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
23dda62
Refactor for the fix_post_row_actions function
Feb 2, 2018
58cabd0
Added visibility declaration for the get_preview_link method
Feb 6, 2018
589a2fa
Changed the arg for the links
Feb 6, 2018
58fa826
Changed the page preview arg to preview_id
Feb 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 2 additions & 17 deletions modules/custom-status/custom-status.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1602,7 +1602,7 @@ function fix_get_sample_permalink_html( $return, $post_id, $new_title, $new_slug
*
* @since 0.8
*/
private function get_preview_link( $post ) {
static function get_preview_link( $post ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that the visibility declaration of methods in this file is not uniform, but we, imho, should follow the best practices declared in the WordPress coding standards project, specifically the WordPress-Extra ruleset, which sets up best practices beyond the core WordPress coding standards.

And thus, the public visibility declaration should be used here. It would also help to highlight the change, as this method becomes public by removing the private visibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review!

I checked the WordPress Coding Standards and I can see that the Methods: Verify that all methods have visibility declared. Does this mean that I should use

public function get_preview_link( $post )

instead of

function get_preview_link( $post )

am I right? I just want to make sure that I follow the Coding Standards and write this the right way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. public static function get_preview_link( $post ) to be exact.


if ( 'page' == $post->post_type ) {
$args = array(
Expand Down Expand Up @@ -1646,22 +1646,7 @@ public function fix_post_row_actions( $actions, $post ) {
if ( empty( $actions['view'] ) )
return $actions;

if ( 'page' == $post->post_type ) {
$args = array(
'page_id' => $post->ID,
);
} else if ( 'post' == $post->post_type ) {
$args = array(
'p' => $post->ID,
);
} else {
$args = array(
'p' => $post->ID,
'post_type' => $post->post_type,
);
}
$args['preview'] = 'true';
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we preserve the preview arg for all the links, not only for the case $post->post_type is 'post'? Looks like it might be safer. The get_preview_link function currently adds it in the above case, while here it's always used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think that it's a good idea. I thought that I already let this arg to preview but I think I rushed with the commit and forgot to check this once again. I'll do the change shortly.

$preview_link = add_query_arg( $args, home_url() );
$preview_link = $this->get_preview_link($post);

$actions['view'] = '<a href="' . esc_url( $preview_link ) . '" title="' . esc_attr( sprintf( __( 'Preview &#8220;%s&#8221;' ), $post->post_title ) ) . '" rel="permalink">' . __( 'Preview' ) . '</a>';
return $actions;
Expand Down