[buddypress-trac] [BuddyPress Trac] #6932: Emails: real unsubscribe functionality
noreply at wordpress.org
Fri Apr 29 13:37:30 UTC 2016
#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
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
* `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/>
More information about the buddypress-trac