Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chainable callbacks #3

Open
maartenJacobs opened this issue May 14, 2012 · 1 comment
Open

Chainable callbacks #3

maartenJacobs opened this issue May 14, 2012 · 1 comment

Comments

@maartenJacobs
Copy link
Contributor

As I was writing wp-notifications-growling, I noticed that we had left out the Notification property callbacks, out of the chainable functions.
In code, that means this is the norm:

$notification = $factory->generateNotification()
                        ->setMessage(htmlspecialchars($message))
                        ->setTo('uids', array($user_id));
$notification->callbacks = $default_callback ? $default_callback : $this->default_callback;
$notification->store();

But in reality, it should have a similarly chainable interface. For instance:

$notification = $factory->generateNotification()
                        ->setMessage(htmlspecialchars($message))
                        ->setTo('uids', array($user_id));
                        ->setCallbacks($default_callback ? $default_callback : $this->default_callback)
                        ->store();

This helps us prevent breaking the chain, and the ease of use.

@maartenJacobs
Copy link
Contributor Author

The same actually counts for updating a notification. The current API allows the following:

$new_message = 'This message is obsolete. Please remove this call!';
foreach ($factory as $notification) {    
      $notification->setMessage($new_message);
      $factory->updateNotification($notification->getId(), $notification);
}

A better approach would be:

$new_message = 'This message is obsolete. Please remove this call!';
foreach ($factory as $notification) {    
      $notification->setMessage($new_message)->save();
}

As every notification has a reference to the factory it is assigned to, we can actually achieve this.
Any thoughts on this, @didlix? I mean, we will have to alter the base modules for the frameworks we support (which is only WordPress for the moment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant