[buddypress-trac] [BuddyPress Trac] #6932: Emails: real unsubscribe functionality

[buddypress-trac]

#6932: Emails: real unsubscribe functionality
----------------------------------------+---------------------------
 Reporter:  DJPaul                      |       Owner:  tharsheblows
     Type:  enhancement                 |      Status:  assigned
 Priority:  normal                      |   Milestone:  2.6
Component:  API - Emails                |     Version:
 Severity:  normal                      |  Resolution:
 Keywords:  has-patch needs-unit-tests  |
----------------------------------------+---------------------------

Comment (by boonebgorges):

 Replying to [comment:13 tharsheblows]:
 > @boonebgorges is this new one about what you mean?
 >
 > >I have a number of comments about implementation details (function
 names and signatures, boring stuff like that)
 >
 > Will you tell me those? Those are the kind of details I'm trying to
 learn. Thank you! :)

 Sure :) I will try to take more time next week to look over the new
 functionality in the patch, but in the meantime, here are some smaller
 requests/comments about [attachment:6932.3.diff], in no particular order:

 * Could use a sweep to ensure adherence to
 https://make.wordpress.org/core/handbook/best-practices/coding-
 standards/php/. Especially: spacing (as between `(int)` and the variable
 being cast), indentation levels, if/then bracket style, trailing commas
 after last array value. See also https://make.wordpress.org/core/handbook
 /best-practices/inline-documentation-standards/php/; in particular, use
 spaces rather than tabs to align columns in docblocks.
 * There are a couple places where I think you can use certain BP functions
 as shortcuts. For example, `bp_get_activity_directory_permalink()` lets
 you skip concatenating the activity URL.
 * Always sanitize content coming from `$_GET`. In most cases, this will
 mean either  `urldecode()` or `intval()`.
 * `bp_email_get_unsubscribe_link()` should have better documentation for
 `$args`. See https://make.wordpress.org/core/handbook/best-practices
 /inline-documentation-standards/php/#1-1-parameters-that-are-arrays
 * `bp_email_get_unsubscribe_link()` - Use more verbose `$args` parameter
 names. 'user_id' and 'notification_type' are much easier to read. The
 short versions are probably acceptable when building URLs, which may have
 length restrictions (and should be obfuscated anyway).
 * You're running URLs through `esc_url()` before sending them to email.
 Are we doing this elsewhere in BP? I think that @DJPaul and I had a
 discussion about it before 2.5, but I don't remember what the result was.
 * The changes to `bp_email_get_type_schema()` are technically a
 compatibility break. I don't know if anyone is using this in the wild, so
 maybe we don't have to worry about it. To be safe, we may have to
 introduce a separate function that contains the full data array (as you
 are currently building it) and then turn `bp_email_get_type_schema()` into
 a wrapper that returns the expected (legacy) value format. Feedback from
 @DJPaul would be helpful here.
 * It looks like you removed `$link` from the signature of
 `bp_email_get_unsubscribe_link()`, but then didn't fix the function :)
 Putting all the paramaters into a single `$args` array - 'link' (or maybe
 better 'redirect_to'), 'notification_type', 'user_id' - seems better to me
 than separating them out into separate arguments.
 * Can we come up with a better function name than `bp_check()`? I know
 it's kinda doing two different things, but we could still have a better
 name than `bp_check()` :)
 * `bp_get_request_unsubscribe` doesn't appear to be hooked to anything. I
 would suggest that `bp_emails_unsubscribe` (also not a great function
 name!) could be hooked to `bp_template_redirect`, or perhaps `bp_actions`.

--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/6932#comment:14>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac