When a subscriber deletes a subscription, an "unsubscribe" request is sent from subscriber to hub. Currently, the module does not handle these POSTs. I will post a patch.

CommentFileSizeAuthor
#7 unsubscribe_1421994_6.patch8.25 KBstevector
#5 unsubscribe_1421994_5.patch8.21 KBstevector
#3 unsubscribe_1421994_3.patch4.91 KBstevector
#1 unsubscribe-1421994-1.patch2.43 KBstevector

Comments

stevector’s picture

stevector
he/him
commented
Status: Active » Needs review
StatusFileSize
new unsubscribe-1421994-1.patch2.43 KB

This patch also switches exit() to drupal_exit().

Crell’s picture

Crell commented
Status: Needs review » Needs work 
+++ b/PuSHHub.inc
@@ -101,16 +101,37 @@ class PuSHHub {
   public function subscribe($post) {

Since the code that follows assumes an array, we should type-specify array here and make it explicit. Any code that breaks is already broken.

+++ b/PuSHHub.inc
@@ -101,16 +101,37 @@ class PuSHHub {
+    if ($post['hub_mode'] == 'subscribe') {

This needs an isset() check to avoid a notice, or else provide a default value.

I would almost say the code would be cleaner by just doing a $post += array_of_empty_strings early on. That's a common tactic.

+++ b/PuSHHub.inc
@@ -101,16 +101,37 @@ class PuSHHub {
+    } else if ($post['hub_mode'] == 'unsubscribe'){

Needs to be broken to a new line.

+++ b/PuSHHub.inc
@@ -101,16 +101,37 @@ class PuSHHub {
+        drupal_exit();

Also, let's move the condition checking up to the menu callback, then have the menu callback fork to either a subscribe() method or an unsubscribe() method. The menu callback can even handle the parsing of $_POST into proper method parameters to make the code more readable and self-documenting rather than passing $_POST around (which is always icky and sometimes a security hole).

stevector’s picture

stevector
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new unsubscribe_1421994_3.patch4.91 KB

This revised patch changes a few things

  • separates subscribe() and unsubscribe methods().
  • Builds up a $passable_post array instead of using $_POST directly. This restricts the array to parameters defined in the spec
  • Moves drupal_exit() to push_hub_endpoint(). I think it makes sense for the page callback to be the function that starts to terminate the request.
Crell’s picture

Crell commented
Status: Needs review » Needs work

Woah, momma, that's some error checking! :-) I definitely like this better.

+++ b/PuSHHub.inc
@@ -98,19 +98,41 @@ class PuSHHub {
    * @param $post

$post should be explicitly documented as an array.

+++ b/PuSHHub.inc
@@ -98,19 +98,41 @@ class PuSHHub {
   public function subscribe($post) {

$post should be explicitly declared as an array, since the code will break otherwise.

Also, since this doesn't really represent a post anymore but the hub command, should it be named differently?

Is there a reason this cannot/should not be made method parameters instead of an array? If it's an array, the legal keys should be documented in the docblock.

+++ b/PuSHHub.inc
@@ -98,19 +98,41 @@ class PuSHHub {
+  public function unsubscribe($post) {

Ibid.

stevector’s picture

stevector
he/him
commented
StatusFileSize
new unsubscribe_1421994_5.patch8.21 KB

I'd like to keep it as an array since outside of Drupal Pubsubhubbub is built on arrays like this. I think it makes the code more accessible.

I have added documentation to the methods. Much of it is copied directly from the spec: http://pubsubhubbub.googlecode.com/svn/trunk/pubsubhubbub-core-0.3.html#...

stevector’s picture

stevector
he/him
commented
Status: Needs work » Needs review
stevector’s picture

stevector
he/him
commented
StatusFileSize
new unsubscribe_1421994_6.patch8.25 KB

Forgot the inline array declaration in the method signatures.

Crell’s picture

Crell commented
Status: Needs review » Reviewed & tested by the community

w00t!

dasjo’s picture

dasjo
he/him
Primary language German
Location Zurich
commented

thank you guys for working on this!

i don't really have time to maintain this module right now, so if you like, feel free to propose yourselfs at #1425142: Looking for (co-)maintainers

Crell’s picture

Crell commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.