Merge pull request #7213 from annando/fix-counter

The delivery counter now counts only successful deliveries

src/Protocol/DFRN.php

@@ -1229,7 +1229,6 @@ class DFRN
 		$curlResult = Network::curl($url);
 
 		if ($curlResult->isTimeout()) {
-			Contact::markForArchival($contact);
 			return -2; // timed out
 		}
 
@@ -1237,29 +1236,24 @@ class DFRN
 
 		$curl_stat = $curlResult->getReturnCode();
 		if (empty($curl_stat)) {
-			Contact::markForArchival($contact);
 			return -3; // timed out
 		}
 
 		Logger::log('dfrn_deliver: ' . $xml, Logger::DATA);
 
 		if (empty($xml)) {
-			Contact::markForArchival($contact);
 			return 3;
 		}
 
 		if (strpos($xml, '<?xml') === false) {
 			Logger::log('dfrn_deliver: no valid XML returned');
 			Logger::log('dfrn_deliver: returned XML: ' . $xml, Logger::DATA);
-			Contact::markForArchival($contact);
 			return 3;
 		}
 
 		$res = XML::parseString($xml);
 
 		if (!is_object($res) || (intval($res->status) != 0) || !strlen($res->challenge) || !strlen($res->dfrn_id)) {
-			Contact::markForArchival($contact);
-
 			if (empty($res->status)) {
 				$status = 3;
 			} else {
@@ -1315,7 +1309,6 @@ class DFRN
 		if ($final_dfrn_id != $orig_id) {
 			Logger::log('dfrn_deliver: wrong dfrn_id.');
 			// did not decode properly - cannot trust this site
-			Contact::markForArchival($contact);
 			return 3;
 		}
 
@@ -1351,7 +1344,6 @@ class DFRN
 
 				default:
 					Logger::log("rino: invalid requested version '$rino_remote_version'");
-					Contact::markForArchival($contact);
 					return -8;
 			}
 
@@ -1391,26 +1383,22 @@ class DFRN
 
 		$curl_stat = $postResult->getReturnCode();
 		if (empty($curl_stat) || empty($xml)) {
-			Contact::markForArchival($contact);
 			return -9; // timed out
 		}
 
 		if (($curl_stat == 503) && stristr($postResult->getHeader(), 'retry-after')) {
-			Contact::markForArchival($contact);
 			return -10;
 		}
 
 		if (strpos($xml, '<?xml') === false) {
 			Logger::log('dfrn_deliver: phase 2: no valid XML returned');
 			Logger::log('dfrn_deliver: phase 2: returned XML: ' . $xml, Logger::DATA);
-			Contact::markForArchival($contact);
 			return 3;
 		}
 
 		$res = XML::parseString($xml);
 
 		if (!isset($res->status)) {
-			Contact::markForArchival($contact);
 			return -11;
 		}
 
@@ -1423,10 +1411,6 @@ class DFRN
 			Logger::log('Delivery returned status '.$res->status.' - '.$res->message, Logger::DEBUG);
 		}
 
-		if (($res->status >= 200) && ($res->status <= 299)) {
-			Contact::unmarkForArchival($contact);
-		}
-
 		return intval($res->status);
 	}
 
@@ -1454,7 +1438,6 @@ class DFRN
 
 				if (empty($contact['addr'])) {
 					Logger::log('Unable to find contact handle for ' . $contact['id'] . ' - ' . $contact['url']);
-					Contact::markForArchival($contact);
 					return -21;
 				}
 			}
@@ -1462,7 +1445,6 @@ class DFRN
 			$fcontact = Diaspora::personByHandle($contact['addr']);
 			if (empty($fcontact)) {
 				Logger::log('Unable to find contact details for ' . $contact['id'] . ' - ' . $contact['addr']);
-				Contact::markForArchival($contact);
 				return -22;
 			}
 			$pubkey = $fcontact['pubkey'];
@@ -1491,26 +1473,22 @@ class DFRN
 		$curl_stat = $postResult->getReturnCode();
 		if (empty($curl_stat) || empty($xml)) {
 			Logger::log('Empty answer from ' . $contact['id'] . ' - ' . $dest_url);
-			Contact::markForArchival($contact);
 			return -9; // timed out
 		}
 
 		if (($curl_stat == 503) && (stristr($postResult->getHeader(), 'retry-after'))) {
-			Contact::markForArchival($contact);
 			return -10;
 		}
 
 		if (strpos($xml, '<?xml') === false) {
 			Logger::log('No valid XML returned from ' . $contact['id'] . ' - ' . $dest_url);
 			Logger::log('Returned XML: ' . $xml, Logger::DATA);
-			Contact::markForArchival($contact);
 			return 3;
 		}
 
 		$res = XML::parseString($xml);
 
 		if (empty($res->status)) {
-			Contact::markForArchival($contact);
 			return -23;
 		}
 
@@ -1518,10 +1496,6 @@ class DFRN
 			Logger::log('Transmit to ' . $dest_url . ' returned status '.$res->status.' - '.$res->message, Logger::DEBUG);
 		}
 
-		if (($res->status >= 200) && ($res->status <= 299)) {
-			Contact::unmarkForArchival($contact);
-		}
-
 		return intval($res->status);
 	}
 

src/Protocol/Diaspora.php

@@ -3110,13 +3110,12 @@ class Diaspora
 	 * @param string $envelope     The message that is to be transmitted
 	 * @param bool   $public_batch Is it a public post?
 	 * @param string $guid         message guid
-	 * @param bool   $no_defer     Don't defer a failing delivery
 	 *
 	 * @return int Result of the transmission
 	 * @throws \Friendica\Network\HTTPException\InternalServerErrorException
 	 * @throws \ImagickException
 	 */
-	public static function transmit(array $owner, array $contact, $envelope, $public_batch, $guid = "", $no_defer = false)
+	private static function transmit(array $owner, array $contact, $envelope, $public_batch, $guid = "")
 	{
 		$enabled = intval(Config::get("system", "diaspora_enabled"));
 		if (!$enabled) {
@@ -3155,20 +3154,6 @@ class Diaspora
 
 		Logger::log("transmit: ".$logid."-".$guid." to ".$dest_url." returns: ".$return_code);
 
-		if (!$return_code || (($return_code == 503) && (stristr($postResult->getHeader(), "retry-after")))) {
-			if (!$no_defer && !empty($contact['contact-type']) && ($contact['contact-type'] != Contact::TYPE_RELAY)) {
-				Logger::info('defer message', ['log' => $logid, 'guid' => $guid, 'destination' => $dest_url]);
-				// defer message for redelivery
-				Worker::defer();
-			}
-
-			// The message could not be delivered. We mark the contact as "dead"
-			Contact::markForArchival($contact);
-		} elseif (($return_code >= 200) && ($return_code <= 299)) {
-			// We successfully delivered a message, the contact is alive
-			Contact::unmarkForArchival($contact);
-		}
-
 		return $return_code ? $return_code : -1;
 	}
 
@@ -3197,13 +3182,12 @@ class Diaspora
 	 * @param array  $message      The message data
 	 * @param bool   $public_batch Is it a public post?
 	 * @param string $guid         message guid
-	 * @param bool   $no_defer     Don't defer a failing delivery
 	 *
 	 * @return int Result of the transmission
 	 * @throws \Friendica\Network\HTTPException\InternalServerErrorException
 	 * @throws \ImagickException
 	 */
-	private static function buildAndTransmit(array $owner, array $contact, $type, $message, $public_batch = false, $guid = "", $no_defer = false)
+	private static function buildAndTransmit(array $owner, array $contact, $type, $message, $public_batch = false, $guid = "")
 	{
 		$msg = self::buildPostXml($type, $message);
 
@@ -3217,7 +3201,7 @@ class Diaspora
 
 		$envelope = self::buildMessage($msg, $owner, $contact, $owner['uprvkey'], $contact['pubkey'], $public_batch);
 
-		$return_code = self::transmit($owner, $contact, $envelope, $public_batch, $guid, $no_defer);
+		$return_code = self::transmit($owner, $contact, $envelope, $public_batch, $guid);
 
 		Logger::log("guid: ".$guid." result ".$return_code, Logger::DEBUG);
 
@@ -4215,9 +4199,10 @@ class Diaspora
 
 		$message = self::createProfileData($uid);
 
+		// @ToDo Split this into single worker jobs
 		foreach ($recips as $recip) {
 			Logger::log("Send updated profile data for user ".$uid." to contact ".$recip["id"], Logger::DEBUG);
-			self::buildAndTransmit($owner, $recip, "profile", $message, false, '', true);
+			self::buildAndTransmit($owner, $recip, "profile", $message);
 		}
 	}
 

src/Worker/Delivery.php

@@ -178,18 +178,10 @@ class Delivery extends BaseObject
 
 			case Protocol::DFRN:
 				self::deliverDFRN($cmd, $contact, $owner, $items, $target_item, $public_message, $top_level, $followup);
-
-				if (in_array($cmd, [Delivery::POST, Delivery::COMMENT])) {
-					Model\ItemDeliveryData::incrementQueueDone($target_id);
-				}
 				break;
 
 			case Protocol::DIASPORA:
 				self::deliverDiaspora($cmd, $contact, $owner, $items, $target_item, $public_message, $top_level, $followup);
-
-				if (in_array($cmd, [Delivery::POST, Delivery::COMMENT])) {
-					Model\ItemDeliveryData::incrementQueueDone($target_id);
-				}
 				break;
 
 			case Protocol::OSTATUS:
@@ -321,21 +313,19 @@ class Delivery extends BaseObject
 
 		Logger::info('DFRN Delivery', ['cmd' => $cmd, 'url' => $contact['url'], 'guid' => defaults($target_item, 'guid', $target_item['id']), 'return' => $deliver_status]);
 
-		if ($deliver_status < 0) {
-			Logger::info('Delivery failed: defer message', ['id' => defaults($target_item, 'guid', $target_item['id'])]);
-			Worker::defer();
-		}
-
 		if (($deliver_status >= 200) && ($deliver_status <= 299)) {
 			// We successfully delivered a message, the contact is alive
 			Model\Contact::unmarkForArchival($contact);
+
+			if (in_array($cmd, [Delivery::POST, Delivery::COMMENT])) {
+				Model\ItemDeliveryData::incrementQueueDone($target_item['id']);
+			}
 		} else {
 			// The message could not be delivered. We mark the contact as "dead"
 			Model\Contact::markForArchival($contact);
 
-			// Transmit via Diaspora when all other methods (legacy DFRN and new one) are failing.
+			Logger::info('Delivery failed: defer message', ['id' => defaults($target_item, 'guid', $target_item['id'])]);
-			// This is a fallback for systems that don't know the new methods.
+			Worker::defer();
-			self::deliverDiaspora($cmd, $contact, $owner, $items, $target_item, $public_message, $top_level, $followup);
 		}
 	}
 
@@ -380,32 +370,49 @@ class Delivery extends BaseObject
 		if (!$contact['pubkey'] && !$public_message) {
 			return;
 		}
+
 		if ($cmd == self::RELOCATION) {
-			Diaspora::sendAccountMigration($owner, $contact, $owner['uid']);
+			$deliver_status = Diaspora::sendAccountMigration($owner, $contact, $owner['uid']);
-			return;
 		} elseif ($target_item['deleted'] && (($target_item['uri'] === $target_item['parent-uri']) || $followup)) {
 			// top-level retraction
 			Logger::log('diaspora retract: ' . $loc);
-			Diaspora::sendRetraction($target_item, $owner, $contact, $public_message);
+			$deliver_status = Diaspora::sendRetraction($target_item, $owner, $contact, $public_message);
-			return;
 		} elseif ($followup) {
 			// send comments and likes to owner to relay
 			Logger::log('diaspora followup: ' . $loc);
-			Diaspora::sendFollowup($target_item, $owner, $contact, $public_message);
+			$deliver_status = Diaspora::sendFollowup($target_item, $owner, $contact, $public_message);
-			return;
 		} elseif ($target_item['uri'] !== $target_item['parent-uri']) {
 			// we are the relay - send comments, likes and relayable_retractions to our conversants
 			Logger::log('diaspora relay: ' . $loc);
-			Diaspora::sendRelay($target_item, $owner, $contact, $public_message);
+			$deliver_status = Diaspora::sendRelay($target_item, $owner, $contact, $public_message);
-			return;
 		} elseif ($top_level && !$walltowall) {
 			// currently no workable solution for sending walltowall
 			Logger::log('diaspora status: ' . $loc);
-			Diaspora::sendStatus($target_item, $owner, $contact, $public_message);
+			$deliver_status = Diaspora::sendStatus($target_item, $owner, $contact, $public_message);
+		} else {
+			Logger::log('Unknown mode ' . $cmd . ' for ' . $loc);
 			return;
 		}
 
-		Logger::log('Unknown mode ' . $cmd . ' for ' . $loc);
+		if (($deliver_status >= 200) && ($deliver_status <= 299)) {
+			// We successfully delivered a message, the contact is alive
+			Model\Contact::unmarkForArchival($contact);
+
+			if (in_array($cmd, [Delivery::POST, Delivery::COMMENT])) {
+				Model\ItemDeliveryData::incrementQueueDone($target_item['id']);
+			}
+		} else {
+			// The message could not be delivered. We mark the contact as "dead"
+			Model\Contact::markForArchival($contact);
+
+			if (!empty($contact['contact-type']) && ($contact['contact-type'] != Model\Contact::TYPE_RELAY)) {
+				Logger::info('Delivery failed: defer message', ['id' => defaults($target_item, 'guid', $target_item['id'])]);
+				// defer message for redelivery
+				Worker::defer();
+			} elseif (in_array($cmd, [Delivery::POST, Delivery::COMMENT])) {
+				Model\ItemDeliveryData::incrementQueueDone($target_item['id']);
+			}
+		}
 	}
 
 	/**