Refactor for the fix_post_row_actions function #442
Conversation
There code from lines 1649 to 1664 are the lines of the get_preview_link function. That's why I removed all this code and replaced it with the call of the get_preview_link. Another change made in this PR is making this get_preview_link method static. The purpose of this function is to change the actions of a post. Also, making it static will let us use it outside this clas at any moment. e Please enter the commit message for your changes. Lines starting
| @@ -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 ) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Right. public static function get_preview_link( $post ) to be exact.
I just added the public visibility for the get_preview_link method according to WordPress Coding Standards.
|
@david-binda Done, I made the change. Could you have another look, please? Thanks! |
| 'post_type' => $post->post_type, | ||
| ); | ||
| } | ||
| $args['preview'] = 'true'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
The preview_id arg from the get_preview_link method is now changed to just preview. Previously, this arg was added only when the $post->post_type was post but I think that it's better to keep the same arg all over this function.
|
@david-binda Can you have another look at this, please? |
| @@ -1620,7 +1620,7 @@ private function get_preview_link( $post ) { | |||
| ); | |||
| } | |||
|
|
|||
| $args['preview_id'] = $post->ID; | |||
There was a problem hiding this comment.
Let's keep the preview_id as it was/is for all links.
| @@ -1620,7 +1620,7 @@ private function get_preview_link( $post ) { | |||
| ); | |||
| } | |||
|
|
|||
| $args['preview_id'] = $post->ID; | |||
| $args['preview'] = $post->ID; | |||
There was a problem hiding this comment.
Let's remove the preview param from line 1614 if we are adding it here (which imho we should)
There was a problem hiding this comment.
I made this change right now. I understand why line 1614 can be removed (it was so obvious once I checked the whole method once again).
The previous arg was preview but it's better to keep it preview_id. Also, I removed the 'preview'=>'true' from the case when the post type is post because this part is set anyway right before the end of the method.
|
@david-binda Do you still have questions regarding this PR or I can mark it for 0.8.3 merge? |
|
@rinatkhaziev @raduconst it still looks like the My understanding of the code says it should be preserved. @raduconst any change you could get into restoring the @rinatkhaziev , feel free to correct me, if my understanding is off. |
|
I'm on my way of checking this out and see what can I do about it. I'll come back with some more info once I look at this once again. |
There code from lines 1649 to 1664 on custom-status.php file is the same as the one from get_preview_link. That's why I replaced it with the call of the get_preview_link.
Another change made in this PR is making this get_preview_link method static. The purpose of this function is to change the actions of a post and it's not related to the class itself. Making it static will let us use it outside this EF_Custom_Status class at any moment.