Make WordPress Core

Opened 13 years ago

Closed 13 years ago

#17976 closed enhancement (fixed)

Make comment_* cookies pluggable

Reported by: pishmishy's profile pishmishy Owned by: westi's profile 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 13 years ago.
Patch makes comment_ cookies pluggable
cookies.diff (2.9 KB) - added by pishmishy 13 years ago.
Patch using actions
17976.patch (2.9 KB) - added by SergeyBiryukov 13 years ago.
17976.2.patch (3.1 KB) - added by SergeyBiryukov 13 years ago.
17976.3.patch (3.2 KB) - added by SergeyBiryukov 13 years ago.
17976.4.patch (3.2 KB) - added by SergeyBiryukov 13 years ago.
17976.5.patch (3.2 KB) - added by SergeyBiryukov 13 years ago.

Download all attachments as: .zip

Change History (29)

@pishmishy
13 years ago

Patch makes comment_ cookies pluggable

#1 @dd32
13 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

#2 @pishmishy
13 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?

#3 @dd32
13 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..

#4 @pishmishy
13 years ago

Thanks. I'll work on a better patch.

#5 @kawauso
13 years ago

  • Cc kawauso added

@pishmishy
13 years ago

Patch using actions

#6 @pishmishy
13 years ago

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

#7 @SergeyBiryukov
13 years ago

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

#8 @pishmishy
13 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.

#9 @westi
13 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.

#10 @SergeyBiryukov
13 years ago

Added a separate action in 17976.2.patch.

#11 follow-ups: @WhiteJV
13 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 13 years ago by WhiteJV (previous) (diff)

#12 in reply to: ↑ 11 @SergeyBiryukov
13 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.

#13 follow-up: @westi
13 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.

#14 in reply to: ↑ 13 @SergeyBiryukov
13 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.

#15 in reply to: ↑ 11 @SergeyBiryukov
13 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 13 years ago by SergeyBiryukov (next)

#16 @jane
13 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.

#17 @nacin
13 years ago

  • Type changed from feature request to enhancement

#18 @ryan
13 years ago

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

#19 @SergeyBiryukov
13 years ago

  • Milestone changed from Future Release to 3.4

#20 @nacin
13 years ago

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

#21 @SergeyBiryukov
13 years ago

Thanks, refreshed the patch.

#22 @westi
13 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.