Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 4 years ago

#31168 closed enhancement (fixed)

Comments should be turned off on pages by default

Reported by: melchoyce's profile melchoyce Owned by: valendesigns's profile valendesigns
Milestone: 4.3 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch needs-testing
Focuses: Cc:

Description

In the vast majority of use cases, you do not want comments turned on for pages. We should turn them off by default and save site builders everywhere some agony.

Attachments (12)

31168-a.patch (1.6 KB) - added by tyxla 9 years ago.
Making comment_status = closed for pages by default. Adding a unit test for that.
31168-b.patch (2.1 KB) - added by tyxla 9 years ago.
Making comment_status = closed and ping_status = closed for pages by default. Adding a unit test for those.
31168-b.2.patch (2.1 KB) - added by tyxla 9 years ago.
Updating 31168-b.patch. This will also set ping_status to closed for the Sample Page that WordPress creates upon installation.
31168_disabling_comment_default.diff (1.1 KB) - added by utkarshd_42 9 years ago.
I changed the default value of Discussion metabox. Which is checked (by default in a new post or page). Dunno whether the best way. But works for me :)
31168.3.diff (1.4 KB) - added by mrahmadawais 9 years ago.
This also disables ping by default as per the patches of tyxla
31168.4.diff (1.6 KB) - added by mrahmadawais 9 years ago.
Ignore the last patch, this one unchecks both the comments and pings inside the metabox when adding a new page
post.diff (573 bytes) - added by fabioneves 9 years ago.
31168.diff (6.3 KB) - added by valendesigns 9 years ago.
31168.2.diff (7.0 KB) - added by valendesigns 9 years ago.
31168.5.diff (6.0 KB) - added by rachelbaker 9 years ago.
31168.6.diff (727 bytes) - added by valendesigns 9 years ago.
31168.7.diff (5.0 KB) - added by valendesigns 9 years ago.

Download all attachments as: .zip

Change History (72)

#1 @SergeyBiryukov
9 years ago

  • Type changed from feature request to enhancement

Related: #12991, #17478, #27111.

#2 @paaljoachim
9 years ago

Hear hear!
Good call..:)

#3 follow-up: @tyxla
9 years ago

Great idea IMHO. I'm not sure why this hasn't been brought so far.

I'm just wondering about the pingbacks/trackbacks (ping_status) - i believe this one should be disabled by default for pages, too? I'm not 100% sure though (second opinion would be needed here).

@tyxla
9 years ago

Making comment_status = closed for pages by default. Adding a unit test for that.

@tyxla
9 years ago

Making comment_status = closed and ping_status = closed for pages by default. Adding a unit test for those.

#4 @tyxla
9 years ago

  • Keywords has-patch added

I've just added 2 patches. The first one (31168-a.patch) makes the default comment_status for pages to be closed. The second one (31168-b.patch) also makes the ping_status for pages closed by default. Both patches include unit tests for testing the default values.

@tyxla
9 years ago

Updating 31168-b.patch. This will also set ping_status to closed for the Sample Page that WordPress creates upon installation.

@utkarshd_42
9 years ago

I changed the default value of Discussion metabox. Which is checked (by default in a new post or page). Dunno whether the best way. But works for me :)

#5 in reply to: ↑ 3 ; follow-up: @melchoyce
9 years ago

Replying to tyxla:

Great idea IMHO. I'm not sure why this hasn't been brought so far.

I'm just wondering about the pingbacks/trackbacks (ping_status) - i believe this one should be disabled by default for pages, too? I'm not 100% sure though (second opinion would be needed here).

For sure, let's disable pingbacks/trackbacks as well by default on pages.

#6 in reply to: ↑ 5 @tyxla
9 years ago

Replying to melchoyce:

Replying to tyxla:

Great idea IMHO. I'm not sure why this hasn't been brought so far.

I'm just wondering about the pingbacks/trackbacks (ping_status) - i believe this one should be disabled by default for pages, too? I'm not 100% sure though (second opinion would be needed here).

For sure, let's disable pingbacks/trackbacks as well by default on pages.

Right, please refer to my 31168-b.2.patch for a solution that includes disabling ping_status for pages.

#7 @wonderboymusic
9 years ago

  • Milestone changed from Awaiting Review to 4.2

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


9 years ago

#9 @wonderboymusic
9 years ago

  • Keywords needs-patch added; has-patch removed
  • Owner set to wonderboymusic
  • Status changed from new to assigned

I would like to shepherd this

#10 follow-up: @rachelbaker
9 years ago

Would a better approach here be to use post_type_supports()? Remove comments from the supports array when the page type is registered.

Sites could re-enable comments with add_post_type_support( 'page', 'comments' );

Using post_type_supports checks is how the REST API will determine what fields to include in endpoint responses.

Last edited 9 years ago by rachelbaker (previous) (diff)

#11 @wonderboymusic
9 years ago

yeah - I working through some helper functions that check post_type_supports()

#12 @melchoyce
9 years ago

How's this going? :)

#13 @helen
9 years ago

Related in terms of post_type_supports(): #28080.

#14 @DrewAPicture
9 years ago

  • Keywords 4.2-beta added

I'd recommend that we probably punt this to a future release. @wonderboymusic still owns it, so that would be his call.

@mrahmadawais
9 years ago

This also disables ping by default as per the patches of tyxla

@mrahmadawais
9 years ago

Ignore the last patch, this one unchecks both the comments and pings inside the metabox when adding a new page

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


9 years ago

#16 @DrewAPicture
9 years ago

  • Keywords 4.2-beta removed
  • Milestone changed from 4.2 to Future Release

Definitely too late to consider this for 4.2. Pushing to a future release.

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


9 years ago

#18 @jorbin
9 years ago

@Wonderboymusic - since you owned this in 4.2, care to deliver us to the promised land in 4.3?

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


9 years ago

#20 @rachelbaker
9 years ago

  • Milestone changed from Future Release to 4.3
  • Owner changed from wonderboymusic to valendesigns

#21 @obenland
9 years ago

I really want to see this happen, but with beta only two weeks away, it might be too late for 4.3 as well.
What is left to do here @valensdesign?

#22 @valendesigns
9 years ago

Sorry, Menu Customizer stole all my time. I'll take a look at this over the weekend.

#23 @obenland
9 years ago

@valensdesign, any news here?

#24 in reply to: ↑ 10 @fabioneves
9 years ago

Replying to rachelbaker:

Would a better approach here be to use post_type_supports()? Remove comments from the supports array when the page type is registered.

Sites could re-enable comments with add_post_type_support( 'page', 'comments' );

Using post_type_supports checks is how the REST API will determine what fields to include in endpoint responses.

This solution from @rachelbaker is much cleaner imo.

@fabioneves
9 years ago

#25 @fabioneves
9 years ago

  • Keywords has-patch added; needs-patch removed

#26 @fabioneves
9 years ago

  • Keywords dev-feedback added

#27 @valendesigns
9 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed

@fabioneves I'm fairly sure we don't want to remove support for comments completely, just turn them off by default for new pages. Removing support drastically changes pre established behavior and would require a developer to turn them back on. I'm going to spend some time on this today and see if I can come up with a solution based on all the comments and Slack discussions.

Last edited 9 years ago by valendesigns (previous) (diff)

@valendesigns
9 years ago

#28 follow-up: @valendesigns
9 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

The latest patch adds two new functions and filters that we can use to get & set the default comment and ping status on a per post_type basis. It also includes four unit tests, two of which would fail before the patch is applied as page & cpt do not support a closed default value when left empty in wp_insert_post or when the post_type does not support comments or trackbacks.

With this patch we can explicitly set the defaults on custom post types with a filter and still allow support for comments and trackbacks on those post types without having them set to open by default. Something I've personally been wanting for a while.

Whomever is still interested in seeing this move forward, please test and let me know your thoughts ASAP.

#29 in reply to: ↑ 28 @efegue
9 years ago

Replying to valendesigns:

The latest patch adds two new functions and filters that we can use to get & set the default comment and ping status on a per post_type basis. It also includes four unit tests, two of which would fail before the patch is applied as page & cpt do not support a closed default value when left empty in wp_insert_post or when the post_type does not support comments or trackbacks.

With this patch we can explicitly set the defaults on custom post types with a filter and still allow support for comments and trackbacks on those post types without having them set to open by default. Something I've personally been wanting for a while.

Whomever is still interested in seeing this move forward, please test and let me know your thoughts ASAP.

I'm going to test this later today, but how is this different from disabling comments from the page? You'll need a developer to turn it back anyway or am I wrong?

#30 follow-ups: @valendesigns
9 years ago

@efegue the previous patch was a global deactivation of comment & trackback support for the page post type. What this patch does is set comment_status & ping_status to closed for new pages and post types that do not support those features. From a UX standpoint you can still check the option to display comments and trackbacks on a new page, so you will not need a developer to get comments back.

However, this patch does allow you to filter the default status for other post types. So a developer would be able to support comments and trackbacks in their custom post type, but have those features default to closed when a new post is created.

From my point of view this change is a lot less destructive, or potentially confusing, to the user. As well, it give us more control as developers to make decisions about our custom post types and how they support comments and trackbacks. And finally, we could standardize the default values with these new functions in other parts of Core.

#31 in reply to: ↑ 30 @efegue
9 years ago

Replying to valendesigns:

@efegue the previous patch was a global deactivation of comment & trackback support for the page post type. What this patch does is set comment_status & ping_status to closed for new pages and post types that do not support those features. From a UX standpoint you can still check the option to display comments and trackbacks on a new page, so you will not need a developer to get comments back.

However, this patch does allow you to filter the default status for other post types. So a developer would be able to support comments and trackbacks in their custom post type, but have those features default to closed when a new post is created.

From my point of view this change is a lot less destructive, or potentially confusing, to the user. As well, it give us more control as developers to make decisions about our custom post types and how they support comments and trackbacks. And finally, we could standardize the default values with these new functions in other parts of Core.

Yup, you're absolutely right. I'll test the patch and let you know how it went. Thanks for the explanation.

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


9 years ago

#33 @valendesigns
9 years ago

The two new functions could potentially be combined into one with a second param to switch cases. Though is being extra DRY the correct approach in this situation if we could potentially loose clarity and what would the function be named if they were combined? Thought I'd put it out there for discussion at the very least.

A separate change we could make is to remove if ( 'page' === $post_type ) { from both functions and create an actual filter function for pages using the newly created filters. A bit of dogfooding in Core.

If anyone has other suggestions or even concerns with this approach please speak up soon as the enhancement deadline is looming.

Last edited 9 years ago by valendesigns (previous) (diff)

#34 in reply to: ↑ 30 @andg
9 years ago

Replying to valendesigns:

@efegue the previous patch was a global deactivation of comment & trackback support for the page post type. What this patch does is set comment_status & ping_status to closed for new pages and post types that do not support those features. From a UX standpoint you can still check the option to display comments and trackbacks on a new page, so you will not need a developer to get comments back.

However, this patch does allow you to filter the default status for other post types. So a developer would be able to support comments and trackbacks in their custom post type, but have those features default to closed when a new post is created.

From my point of view this change is a lot less destructive, or potentially confusing, to the user. As well, it give us more control as developers to make decisions about our custom post types and how they support comments and trackbacks. And finally, we could standardize the default values with these new functions in other parts of Core.

+1

I was wondering, looking at the patch you've submitted, wouldn't the get_default_comment_status and get_default_ping_status filters be more clear if they had a $post_type parameter, otherwise one would be forced to use a global variable (correct me if I'm wrong).

Alternatively, the information about the post type being filtered could be included in the filter name itself, such as get_default_comment_status_$post_type.

In any case, this is a good solution to a very common problem, so thanks.

#35 follow-up: @valendesigns
9 years ago

@andg I forgot to add the $post_type var to the filters. Duh! Nice catch lol.

#36 in reply to: ↑ 35 @andg
9 years ago

Replying to valendesigns:

@andg I forgot to add the $post_type var to the filters. Duh! Nice catch lol.

Glad to help ;)

@valendesigns
9 years ago

#37 @valendesigns
9 years ago

The latest patch combines the two functions and adds a proper filter.

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


9 years ago

@rachelbaker
9 years ago

#39 @rachelbaker
9 years ago

Moved the logic for the different comment types into one public function get_default_comment_status that uses a new $comment_type param. Unit tests all pass.

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


9 years ago

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


9 years ago

#42 @obenland
9 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 33041:

Turn of comments for pages by default.

Pages are static content, for which comments are not expected out of the box.

Props valendesigns, rachelbaker.
Fixes #31168.

#43 @helen
9 years ago

We should write this up on Make/Core, especially given the change of behavior for new posts - we should be open to needing to change that default case.

#44 @valendesigns
9 years ago

@rachelbaker Thank you for the assist. Cheers!

#45 @melchoyce
9 years ago

Thanks everyone for making this happen. :) I am stoked!

#46 follow-up: @helen
9 years ago

  • Keywords needs-patch 2nd-opinion added; has-patch needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

As I was working with Mel on the Make/Core post, I noticed that the hook ended up with a dynamic name: get_{$post_type}_default_comment_status. Dynamic hooks are neat, but not always easy to find, and there's usually a case to be made for the generic non-dynamic version. I think it'd be better to go with get_default_comment_status and pass the post type as context.

Also, do we need to do a general audit of get_option( 'default_comment_status' ) uses?

#47 in reply to: ↑ 46 @obenland
9 years ago

Replying to helen:

I noticed that the hook ended up with a dynamic name: get_{$post_type}_default_comment_status. Dynamic hooks are neat, but not always easy to find, and there's usually a case to be made for the generic non-dynamic version. I think it'd be better to go with get_default_comment_status and pass the post type as context.

I was actually thinking about that in my final review. I think we should definitely do that.

Also, do we need to do a general audit of get_option( 'default_comment_status' ) uses?

Yes we do.

#48 @valendesigns
9 years ago

I'll be at my desk shortly and will create a new patch.

@valendesigns
9 years ago

#49 follow-up: @valendesigns
9 years ago

  • Keywords has-patch added; needs-patch removed

The only place left in Core that uses default_comment_status and default_ping_status is in class-wp-xmlrpc-server.php and I'm not sure the best way to test the changes made to that file so I'm hesitant to make changes I can't verify. I've uploaded a patch that fixes the filter though.

#50 @obenland
9 years ago

Would we not replace all instanced of get_option( 'default_comment_status' ) and get_option( 'default_ping_status' )? There are a bit more of those.

Last edited 9 years ago by obenland (previous) (diff)

#51 in reply to: ↑ 49 @rachelbaker
9 years ago

Looks like they are also used in redirect_post() and get_default_post_to_edit()

Replying to valendesigns:

The only place left in Core that uses default_comment_status and default_ping_status is in class-wp-xmlrpc-server.php and I'm not sure the best way to test the changes made to that file so I'm hesitant to make changes I can't verify. I've uploaded a patch that fixes the filter though.

#52 @valendesigns
9 years ago

I must have been doing a search earlier from inside the wp-includes directory and not from the root because I see a bunch more of them now. I'm replacing and running the unit tests. I'll upload a new patch soon.

@valendesigns
9 years ago

#53 @valendesigns
9 years ago

  • Keywords needs-testing added; 2nd-opinion removed

I've switched over in various places to the new function and cleaned up the filter in 31168.7.diff. Unit test all run as expected.

#54 @obenland
9 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 33054:

Use get_default_comment_status() globally.

Also makes the filter name static and passes the post type for context.

Props valendesigns.
Fixes #31168.

This ticket was mentioned in Slack in #themereview by poena. View the logs.


9 years ago

#56 @nacin
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Is there a compelling reason for the filter to be a dynamic one in the form of get_{$post_type}_default_comment_status? I think this is more appropriate as apply_filters( "get_default_comment_status", $status, $post_type, $comment_type ).

We typically force specific, dynamic filters on developers in a few situations (these somewhat overlap/blur):

  • when we find it to be overly generic, such as option (does not exist) vs option_{$option}
  • when we find it to be overly dangerous, such as update_option (incorrectly using this filter took down .com, thus it was reverted there and #13836 was wontfix'd -- it was only added four years later when there was a compelling use case)
  • when it'd be significantly easier to use the dynamic one, such as when it's used for manage_{$taxonomy}_custom_column
  • when we use it as the underlying piece of an API, such as sanitize_option_{$option} (which is what register_setting() directly uses) or auth_post_meta_{$meta_key} (ditto for register_meta())

For something so minor, it seems annoying to need to add_filter() for each affected post type you may be trying to control. It's also a bit weird that it's broken down by *post* type and not (also?) by *comment* type, which is probably more expected here. I don't think there is a worry that someone will accidentally override the default comment status everywhere, unless that's actually exactly what they intend to do. As I write this, I wonder if this is worth keeping like this to avoid people stomping on random custom post types. But, most of our filters with a $post_type variable use it as an argument, and not as part of the dynamic name.

P.S. Could someone perhaps massage the above guidance into the handbook? :-D

#57 @nacin
9 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Re-closing as now I see that this thought process was already laid out above, initially by @helen (and changed in [33054]), and I just missed it. This probably does belong in the handbook somewhere if anyone wants to take a swing. :-)

A few more thoughts on when to avoid dynamic hook names:

  • When the static part of the dynamic hooks is so generically named that it can conflict with other dynamic (or static) hooks. This is a huge problem with some older hooks, but we're more wordy these days. (Not a major problem on this one.)
  • As @helen said, it can be harder to find the hook, and also harder to take an add_filter() call in code you're reading and understand exactly what the actual filter is. (This could be a cool reverse-engineering tool to write...)

#58 @nacin
9 years ago

And finally, variables in a hook name should probably only ever be used at the end of the hook these days ("foo_bar_{$baz}", not "foo_{$bar}_baz"), to ease in finding things and to make it easier to avoid conflicts. I recognize many existing hooks in WordPress violate this.

#59 @helen
9 years ago

In 33122:

Move get_default_comment_status() to wp-includes/comment.php to sit alongside get_comment_statuses().

props nacin.
see #31168.

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


4 years ago

Note: See TracTickets for help on using tickets.