Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#39650 closed defect (bug) (fixed)

Commenting on a draft post results in blank page

Reported by: sumitbagthariya16's profile sumitbagthariya16 Owned by: johnbillion's profile johnbillion
Milestone: 4.8 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Hi,

When I submit the comment on draft post it will redirect to the blank page. I have attached video for the more information.

Thanks

Attachments (11)

issue with draft post.webm (1019.2 KB) - added by sumitbagthariya16 7 years ago.
39650.patch (529 bytes) - added by sagarprajapati 7 years ago.
39650-1.diff (634 bytes) - added by milindmore22 7 years ago.
Patch will not show comment form if post status is not publish. The patch will also take care of pending status.
39650.2.patch (596 bytes) - added by sagarprajapati 7 years ago.
Followed the standards
39650.3.patch (599 bytes) - added by sagarprajapati 7 years ago.
39650-2.diff (637 bytes) - added by milindmore22 7 years ago.
Updated patch by passing $post_id as get_post_status parameter
39650-3.diff (568 bytes) - added by milindmore22 7 years ago.
added after post_id check as per comment 7
39650.4.patch (530 bytes) - added by mayurk 7 years ago.
39650.5.patch (2.2 KB) - added by sagarprajapati 7 years ago.
39650.6.patch (676 bytes) - added by sagarprajapati 7 years ago.
39650.diff (2.3 KB) - added by swissspidy 7 years ago.

Download all attachments as: .zip

Change History (32)

#1 @bhaveshkhadodara
7 years ago

Hi @sumitbagthariya16,

Here we should remove the comment box when the post is draft.

Thanks

#2 @sagarprajapati
7 years ago

  • Keywords has-patch added

Hi @bhaveshkhadodara , @sumitbagthariya16

I have added patch with above issue solution.

Thanks

@milindmore22
7 years ago

Patch will not show comment form if post status is not publish. The patch will also take care of pending status.

This ticket was mentioned in Slack in #core by sagarprajapati. View the logs.


7 years ago

@sagarprajapati
7 years ago

Followed the standards

#4 @SergeyBiryukov
7 years ago

  • Milestone changed from Awaiting Review to 4.8

This ticket was mentioned in Slack in #core by sumitbagthariya16. View the logs.


7 years ago

#6 @sagarprajapati
7 years ago

Updated $post to $post_id

@milindmore22
7 years ago

Updated patch by passing $post_id as get_post_status parameter

#7 @dhanendran
7 years ago

@sagarprajapati It's better to have this condition after the first `if` condition. Because there might be some chances the $post_id will be null.

if ( null === $post_id )
    $post_id = get_the_ID();

@milindmore22
7 years ago

added after post_id check as per comment 7

#8 @milindmore22
7 years ago

@sagarprajapati same thing happens when we have pending status my patch will show comment form only if status is publish.
@dhanendran thanks for suggestions!

#9 @swissspidy
7 years ago

  • Keywords needs-patch added; has-patch removed
  • Summary changed from Display blank page when post status is draft to Commenting on a draft post results in blank page
  • Version 4.7.1 deleted

What @dhanendran said. Also, the code would need somebrackets to follow the WordPress PHP coding standards.

However, I don't think hiding the comment form for draft posts is the ideal solution here. Allowing comments on drafts might be desirable under some circumstances (live previews in the customizer, REST API, etc. See also #19739).

Instead of a blank page there should be an actual error message. This means changing wp_handle_comment_submission() accordingly, where there's currently this line: return new WP_Error( 'comment_on_draft' ); (note the lack of error message).

#10 @swissspidy
7 years ago

@milindmore22 Please note that your patch would break commenting on private posts.

#11 @sagarprajapati
7 years ago

@swissspidy Thank you so much. This would be great idea.

@mayurk
7 years ago

#12 @mayurk
7 years ago

  • Keywords has-patch added; needs-patch removed

Hi @swissspidy

I agree with you. I have prepared a patch with your suggestions. Please check it.

Thanks

#13 @sagarprajapati
7 years ago

Hi @swissspidy and @mayurk

@mayurk: I also working on this issue. I agreed your patch but we will need to add the proper error message as per swissspidy's suggestions.

I have added the patch with the error message. Please check it.

Thanks

#14 @swissspidy
7 years ago

  • Keywords needs-unit-tests dev-feedback added

Seems like you guys should work together / coordinate on who's updating a patch :-)

39650.4.patch is not needed. That would be to big of a change. Adding the message argument to the WP_Error object automatically displays that message when posting the comment. That's why the exit; there is just fine as-is.

39650.5.patch does not need to touch wp-comments-post.php either. Also, why adding the error message to every WP_Error object and not just on comment_on_draft (the original scenario described in the ticket)?

The error message should probably only be shown if the user has the capability to read the post, so a current_user_can() check seems to be needed.

#15 @sagarprajapati
7 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

Sorry @swissspidy

I thought that it is not working for all others. I have updated patch with the changes. Please check it.

I have also done unit test for the above patch and here is the result.

C:\wamp\www\wordpress-core>phpunit --group comment
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 5.7.5 by Sebastian Bergmann and contributors.

..............................................................  62 / 339 ( 18%)
.............................................................. 124 / 339 ( 36%)
.............................................................. 186 / 339 ( 54%)
.............................................................. 248 / 339 ( 73%)
.............................................................. 310 / 339 ( 91%)
.............................                                  339 / 339 (100%)

Time: 50.03 seconds, Memory: 90.00MB

OK (339 tests, 657 assertions)

Thank you.

#16 @swissspidy
7 years ago

  • Keywords needs-unit-tests added; has-unit-tests removed

needs-unit-tests means some unit tests need to be written for this, not only running the existing tests. See the handbook for a list of all trac keywords. Let me know if I can help with that. I might have time to write tests myself next week.

39650.6.patch goes into the right direction. The correct capability might be edit_post instead of read_post, but we'll see that with the unit test.

#17 follow-up: @milindmore22
7 years ago

Great work with patch @sagarprajapati, Thanks @swissspidy totally forgot that will keep in mind about flow
Guys #19739 does this ticket related?

#18 in reply to: ↑ 17 @swissspidy
7 years ago

Replying to milindmore22:

Great work with patch @sagarprajapati, Thanks @swissspidy totally forgot that will keep in mind about flow
Guys #19739 does this ticket related?

Yep, it's related and I mentioned it before. Let's see if we can fix both at once.

@swissspidy
7 years ago

#19 @swissspidy
7 years ago

  • Keywords has-unit-tests added; dev-feedback needs-unit-tests removed

The correct capability might be edit_post instead of read_post, but we'll see that with the unit test.

read_post is fine. map_meta_cap() handles this perfectly.

#20 @johnbillion
7 years ago

  • Owner set to johnbillion
  • Resolution set to fixed
  • Status changed from new to closed

In 40128:

Comments: When commenting on a draft post, display a friendly error message if the user can view the post.

This prevents the unhelpful white screen of death when a user who can view the post (eg. preview it) leaves a comment while the post is in draft.

Props sagarprajapati, milindmore22, mayurk, swissspidy
Fixes #39650

#21 @johnbillion
7 years ago

#40689 was marked as a duplicate.

Note: See TracTickets for help on using tickets.