[buddypress-trac] [BuddyPress] #5148: Make notifications a separate component
buddypress-trac
noreply at wordpress.org
Sat Sep 7 01:13:33 UTC 2013
#5148: Make notifications a separate component
----------------------------------+------------------------------
Reporter: johnjamesjacoby | Owner: johnjamesjacoby
Type: defect (bug) | Status: new
Priority: normal | Milestone: 2.0
Component: Notifications | Version:
Severity: normal | Resolution:
Keywords: needs-patch needs-ui |
----------------------------------+------------------------------
Comment (by boonebgorges):
Patch looks really good, r-a-y. I'm a big fan of moving the miscellaneous
methods into central `delete()` and `get()` methods. Also, the fact that
you fixed a couple of these basic but major bugs is crazy to me - I guess
that means that few people are hacking on Notifications at the moment.
(Heck, I've hardly ever hacked on it.)
Couple small comments:
- Since we're essentially gutting a couple of user facing functions and
replacing them with the new all-purpose `get` and `delete` methods, we
should have unit tests for the procedural wrappers. Then we can rest
assured that the refactor didn't break anything.
- `get_where_sql()` looks good to me, but let's mark it `protected
static`. It's probably a bad idea to encourage developers to use BP's
utility methods for building partial SQL statements, as it binds our hands
if we ever want to make internal syntax modifications.
But yeah, these look like very solid improvements for 1.9.
--
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/5148#comment:4>
BuddyPress <http://buddypress.org/>
BuddyPress
More information about the buddypress-trac
mailing list