PuSH: verification fails the first time

That's strange. Looks like a bug to me. You don't notice any skipped feed items?

What hub are you using?

Thank you -

Unfortunately I don't have time to look into this. But next actions would be to meticulously log all interactions between Feeds and push.appspot.com and see where the glitch happens. Possibly this involves deploying the [PuSH reference hub](http://code.google.com/p/pubsubhubbub/)

I think I'm seeing the same thing in the 7.x version. After some debugging it looks like the PuSHSubscription::load() called from PuSHSubscriber::subscription() is not finding the record that was just inserted. It's possible that this is a race condition; however, drupal_write_record in PushSubscription::save() is returning SAVED_NEW.

This definitely appears to be a race condition. By adding some semaphores in the load and save functions I've been able to determine that the verify request is happening before the subscription information is saved to the feeds_push_subscriptions table.

I think I know what's going on here: if you're using a feed node node_save will create a transaction. Which means that the feeds information will not have been written to the database when the subscription request is sent to the PuSH hub. Since PHP is blocking the thread while it waits for the subscription request to finish the feed info is still not existent when the PuSH hub calls back and everything falls over. This would also explain why it's not happening for stand-alone form importers.

After some discussion with others that are seeing this problem we decided to move the subscription workflow to hook_exit(). I've attached a patch for a first-cut attempt at this.

Here's another cut at this that only uses hook_exit to run the subscribe code if this is on a feeds node save.

This issue is somewhat related to #1418382: Fix unsubscribe from PuSH hub for feeds nodes.

+++ b/feeds.module
@@ -398,6 +398,16 @@ function feeds_exit() {
+  if ($subscriptions = &drupal_static('feeds_pubsubhubbub_subscribe_jobs', array())) {

I don't think drupal_static() is appropriate here. It's not intended as a data sharing mechanism; it's more for emergency cache clearing. Rather, it should be using a collector function. (Think drupal_get_title() or drupal_set_message().)

Also, pedantic point, if the collector function returns an empty array for nothing then you don't need an if statement. The foreach() will simply not run for an empty array anyway, which is a bit cleaner.

+++ b/feeds.module
@@ -398,6 +398,16 @@ function feeds_exit() {
+    // Empty out the processed array.
+    $subscriptions = array();

This is a one-process variable anyway, so there's no need to zero-out the array. It will be zeroed out in a few milliseconds when the process ends anyway.

Here's another attempt at this that's more inline with drupal_set_message()/ drupal_set_title() and I put in a little bit of cleanup overall to simplify the logic for when to move subscriptions to hook_exit().

+++ b/feeds.module
@@ -1006,3 +1015,30 @@ function feeds_valid_url($url, $absolute = FALSE) {
+ * @param {array} $job
+ *   Information about a new job to queue up.
+ *
+ * @return {array}.

Drupal coding standards say to not use {} around the type of a variable in a docblock. That's not necessary in Doxygen or PHPDoc.

+++ b/feeds.module
@@ -1006,3 +1015,30 @@ function feeds_valid_url($url, $absolute = FALSE) {
+ * @return {array}.

Ibid.

Looks good otherwise. Technically the collector could be folded into a single function, but I don't really care either way. :-)

Done.

I'm all for fewer LOC, but semantically it made more sense to me to have a getter and setter, even if the getter just calls the setter with no arguments.

Looks good to me.

Committed #16 with some code style cleanups to 7.x-2.x. http://drupalcode.org/project/feeds.git/commit/cd8513e

Likely needs to be backported to 6.x-1.x as well.

AFAIK, there are no transactions in D6.