WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#17976 closed enhancement (fixed)

Make comment_* cookies pluggable

Reported by: pishmishy Owned by: westi
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.2
Component: Comments Keywords: has-patch early
Focuses: Cc:

Description

EU legislation dictates that some particular uses of cookies must first require consent from the user. The exact implementation of the EU legislation is dependent upon the individual member states and so there are a number of conflicting and confusing ideas over what types of cookies can be used with and without consent. See http://en.wikipedia.org/wiki/Directive_on_Privacy_and_Electronic_Communications for more details.

The functionality provided by the comment_ cookies set by WordPress is arguably not required for a WordPress site to function and in some parts of the EU consent may be required. Regardless of your view on the sensibilities of this we should allow site administrators to override the default behavior with a plugin if necessary.

(I am not proposing that the default behavior of WordPress be modified in any form in order to meet the requirements of any of this confusing legislation. See the forums for debates about this).

I'm attaching a patch that makes the calls to set_cookie from wp-comment-post.php pluggable. I've also got an example plugin, please let me know if posting it would be helpful.

Attachments (7)

17976.diff (2.3 KB) - added by pishmishy 4 years ago.
Patch makes comment_ cookies pluggable
cookies.diff (2.9 KB) - added by pishmishy 4 years ago.
Patch using actions
17976.patch (2.9 KB) - added by SergeyBiryukov 4 years ago.
17976.2.patch (3.1 KB) - added by SergeyBiryukov 4 years ago.
17976.3.patch (3.2 KB) - added by SergeyBiryukov 3 years ago.
17976.4.patch (3.2 KB) - added by SergeyBiryukov 3 years ago.
17976.5.patch (3.2 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (29)

@pishmishy4 years ago

Patch makes comment_ cookies pluggable

comment:1 @dd324 years ago

Whilst it adds a set-cookie to the response still, returning a negitive value on the comment_cookie_lifetime filter should also comply, as a browser would not store the cookie, and would delete any existing cookies.

However, we should avoid adding pluggable functions, and instead should alter the process to perform the cookie-adding routines in a function hooked to an action.

See Also: #16979

comment:2 @pishmishy4 years ago

I was going down the action path before I wrote this patch but then changed my mind. I'm probably wrong :)

I think what put me off is that I couldn't remember how to make sure that my test plugin had the only function registered for that action. Is it just a case of calling remove_all_actions() and hoping that another plugin doesn't come along after it?

comment:3 @dd324 years ago

as an example, all I'd do is:

in default-filters:
add_action('commented_on_post', 'add_commenter_cookies'); (need to check that action)

in a plugin on an appropriate hook (probably init?)
remove_action('commented_on_post', 'add_commenter_cookies');

and put the add_commenter_cookies function into the comments include file..

comment:4 @pishmishy4 years ago

Thanks. I'll work on a better patch.

comment:5 @kawauso4 years ago

  • Cc kawauso added

@pishmishy4 years ago

Patch using actions

comment:6 @pishmishy4 years ago

Something like the attached? I can't quite get it working though - not sure what I'm doing wrong.

@SergeyBiryukov4 years ago

comment:7 @SergeyBiryukov4 years ago

Revised pishmishy's previous patch. Added missing global variable and adjusted to comply with our coding standards.

comment:8 @pishmishy4 years ago

Thank you - I knew I was missing something.

I've been able to write a plugin to work with this patch and it appears to work well. I'm asking some colleagues to test the patch and my plugin to see if it allows them to control the use of cookies in the ways they'd like to, whilst still retaining the default behavior.

comment:9 @westi4 years ago

I don't think using the comment_post action is correct here we should instead have an action which is specific to the wp-comment-posts.php script for setting the cookies and then plugins can just do a simple remove action call to filter out the cookie setting.

@SergeyBiryukov4 years ago

comment:10 @SergeyBiryukov4 years ago

Added a separate action in 17976.2.patch.

comment:11 follow-ups: @WhiteJV3 years ago

If we were to wish to not set cookies would the following work from within mu-plugins?

remove_action( 'set_comment_cookies', 'set_comment_cookies' );

If so, this will do well to allow us to not set new cookies, however, due to the long TTL of the cookie, there is a potential to expose personally identifiable information of users who have commented in the past but have never flushed their cookies. Could this patch be expanded to also include a pluggable function for reading comment cookies?

Last edited 3 years ago by WhiteJV (previous) (diff)

@SergeyBiryukov3 years ago

comment:12 in reply to: ↑ 11 @SergeyBiryukov3 years ago

Replying to WhiteJV:

If we were to wish to not set cookies would the following work from within mu-plugins?

Yes, that's the idea. Refreshed the patch for current trunk.

comment:13 follow-up: @westi3 years ago

  • Keywords westi-likes added
  • Milestone changed from Awaiting Review to 3.3
  • Owner set to westi
  • Status changed from new to accepted

Thanks for the updated patch

I think it would be nicer to pass the 'user' object to the action and therefore the function rather than relying on the name of a global so that the cookie setting is independent of any globals.

@SergeyBiryukov3 years ago

comment:14 in reply to: ↑ 13 @SergeyBiryukov3 years ago

Replying to westi:

I think it would be nicer to pass the 'user' object to the action and therefore the function rather than relying on the name of a global so that the cookie setting is independent of any globals.

Thanks for the feedback, done in 17976.4.patch.

Would it make sense to pass the whole comment object as well, rather than call get_comment() for the second time? If no, I'll replace it with $comment_id again.

comment:15 in reply to: ↑ 11 @SergeyBiryukov3 years ago

Replying to WhiteJV:

If so, this will do well to allow us to not set new cookies, however, due to the long TTL of the cookie, there is a potential to expose personally identifiable information of users who have commented in the past but have never flushed their cookies. Could this patch be expanded to also include a pluggable function for reading comment cookies?

It's possible to hook into wp_get_current_commenter() function to ignore existing cookies:

function ignore_existing_comment_cookies($commenter) {
	return array( 'comment_author' => '', 'comment_author_email' => '', 'comment_author_url' => '' );
}
add_filter( 'wp_get_current_commenter', 'ignore_existing_comment_cookies' );
Version 0, edited 3 years ago by SergeyBiryukov (next)

comment:16 @jane3 years ago

@westi: It seems awfully late to be trying to squeeze this in, as we're already in 2nd beta, on the march toward RC. By our own definitions of each phase, this should be bumped to 3.4-early.

comment:17 @nacin3 years ago

  • Type changed from feature request to enhancement

comment:18 @ryan3 years ago

  • Keywords early added; westi-likes removed
  • Milestone changed from 3.3 to Future Release

comment:19 @SergeyBiryukov3 years ago

  • Milestone changed from Future Release to 3.4

comment:20 @nacin3 years ago

17976.4.patch looks good. Two suggestions: Bail early if $user->ID, and prefix function (though not action) with wp_.

@SergeyBiryukov3 years ago

comment:21 @SergeyBiryukov3 years ago

Thanks, refreshed the patch.

comment:22 @westi3 years ago

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

In [19622]:

Make it possible for commenter cookies to be disabled if someone wants to by setting them on an action instead of always. Fixes #17976 props SergeyBiryukov and pishmishy .

Note: See TracTickets for help on using tickets.