feat(river): elgg_delete_river checks permissions and fires events

fixes: Elgg#10117

actions/avatar/crop.php

@@ -31,14 +31,17 @@
 }
 
 system_message(elgg_echo('avatar:crop:success'));
+
+// River
 $view = 'river/user/default/profileiconupdate';
-_elgg_delete_river(['subject_guid' => $owner->guid, 'view' => $view]);
+// remove old river items
+elgg_delete_river(['subject_guid' => $owner->guid, 'view' => $view, 'limit' => false]);
+// create new river entry
 elgg_create_river_item([
 	'view' => $view,
 	'action_type' => 'update',
 	'subject_guid' => $owner->guid,
 	'object_guid' => $owner->guid,
 ]);
 
-
 forward(REFERER);

actions/avatar/upload.php

@@ -24,9 +24,12 @@
 
 if (elgg_trigger_event('profileiconupdate', $owner->type, $owner)) {
 	system_message(elgg_echo("avatar:upload:success"));
-
+
+	// River
 	$view = 'river/user/default/profileiconupdate';
-	_elgg_delete_river(['subject_guid' => $owner->guid, 'view' => $view]);
+	// remove old river items
+	elgg_delete_river(['subject_guid' => $owner->guid, 'view' => $view, 'limit' => false]);
+	// create new river entry
 	elgg_create_river_item([
 		'view' => $view,
 		'action_type' => 'update',

docs/guides/hooks-list.rst

@@ -321,8 +321,6 @@ Permission hooks
 	Return boolean for if the user ``$params['user']`` can delete the river item ``$params['item']``. Defaults to
 	``true`` for admins and ``false`` for other users.
 
-	.. note:: This check is not performed when using the deprecated ``elgg_delete_river()``.
-
 **permissions_check:download, file**
 	Return boolean for if the user ``$params['user']`` can download the file in ``$params['entity']``.
 

docs/guides/upgrading.rst

@@ -571,6 +571,16 @@ context. They still clear floats and allow breaking words to wrap text.
 Core modules and layouts that relied on space-filling have been reworked for Flexbox and
 we encourage devs to do the same, rather than use the problematic ``overflow: hidden``.
 
+Delete river items
+------------------
+
+The function ``elgg_delete_river()`` which was deprecated in 2.3, has been reinstated. Notable changes between the internals of this function are;
+
+ * It accepts all ``$options`` from ``elgg_get_river()`` but requires at least one of the following params to be set id(s), annotation_id(s), subject_guid(s), object_guid(s), target_guid(s) or view(s)
+ * Since ``elgg_get_river`` by default has a limit on the number of river items it fetches, if you wish to remove all river items you need to set ``limit`` to ``false``
+ * A hook is fired for each river item which checks the delete permissions 
+ * Events are fired just before and after a river item has been deleted
+
 From 2.2 to 2.3
 ===============
 

engine/classes/Elgg/Application.php

@@ -174,7 +174,6 @@ public function loadCore() {
 			'widgets.php',
 
 			// backward compatibility
-			'deprecated-2.1.php',
 			'deprecated-3.0.php',
 		];
 

engine/classes/Elgg/UserCapabilities.php

@@ -136,8 +136,6 @@ public function canDelete(ElggEntity $entity, $user_guid = 0) {
 	 *
 	 * @tip Can be overridden by registering for the "permissions_check:delete", "river" plugin hook.
 	 *
-	 * @note This is not called by elgg_delete_river().
-	 *
 	 * @param ElggRiverItem $item      River item
 	 * @param int           $user_guid The user GUID, optionally (default: logged in user)
 	 *

engine/classes/ElggAnnotation.php

@@ -77,7 +77,7 @@ public function save() {
 	public function delete() {
 		$result = _elgg_delete_metastring_based_object_by_id($this->id, 'annotation');
 		if ($result) {
-			_elgg_delete_river(['annotation_id' => $this->id]);
+			elgg_delete_river(['annotation_id' => $this->id, 'limit' => false]);
 		}
 
 		return $result;

engine/classes/ElggEntity.php

@@ -1937,9 +1937,10 @@ public function delete($recursive = true) {
 		access_show_hidden_entities($entity_disable_override);
 		elgg_set_ignore_access($ia);
 
-		_elgg_delete_river(['subject_guid' => $guid]);
-		_elgg_delete_river(['object_guid' => $guid]);
-		_elgg_delete_river(['target_guid' => $guid]);
+		elgg_delete_river(['subject_guid' => $guid, 'limit' => false]);
+		elgg_delete_river(['object_guid' => $guid, 'limit' => false]);
+		elgg_delete_river(['target_guid' => $guid, 'limit' => false]);
+
 		remove_all_private_settings($guid);
 
 		_elgg_invalidate_cache_for_entity($guid);

engine/classes/ElggRiverItem.php

@@ -136,8 +136,6 @@ public function getSubtype() {
 	 *
 	 * @tip Can be overridden by registering for the "permissions_check:delete", "river" plugin hook.
 	 *
-	 * @note This is not called by elgg_delete_river().
-	 *
 	 * @param int $user_guid The user GUID, optionally (default: logged in user)
 	 *
 	 * @return bool Whether this river item should be considered deletable by the given user.

engine/lib/elgglib.php

@@ -1748,7 +1748,7 @@ function elgg_batch_delete_callback($object) {
  * potentially dangerous operations.
  *
  * @param array  $options Options array
- * @param string $type    Options type: metadata or annotation
+ * @param string $type    Options type: metadata, annotation or river
  * @return bool
  * @access private
  */
@@ -1785,6 +1785,18 @@ function _elgg_is_valid_options_for_batch_operation($options, $type) {
 			$required = array_merge($required, $annotations_required);
 			break;
 
+		case 'river':
+			// overriding generic restraints as guids isn't supported in river
+			$required = [
+				'id', 'ids',
+				'subject_guid', 'subject_guids',
+				'object_guid', 'object_guids',
+				'target_guid', 'target_guids',
+				'annotation_id', 'annotation_ids',
+				'view', 'views',
+			];
+			break;
+
 		default:
 			return false;
 	}

engine/lib/river.php

@@ -371,6 +371,32 @@ function elgg_get_river(array $options = []) {
 	}
 }
 
+/**
+ * Delete river items based on $options.
+ *
+ * @warning Unlike elgg_get_river() this will not accept an empty options array!
+ *          This requires at least one constraint: id(s), annotation_id(s)
+ *          subject_guid(s), object_guid(s), target_guid(s)
+ *          or view(s) must be set.
+ *
+ * @param array $options An options array. {@link elgg_get_river()}
+ *
+ * @return bool|null true on success, false on failure, null if no metadata to delete.
+ *
+ * @since 1.8.0
+ */
+function elgg_delete_river(array $options = []) {
+
+	if (!_elgg_is_valid_options_for_batch_operation($options, 'river')) {
+		// requirements not met
+		return false;
+	}
+
+	$batch = new ElggBatch('elgg_get_river', $options, 'elgg_batch_delete_callback', 25, false);
+
+	return $batch->callbackResult;
+}
+
 /**
  * Prefetch entities that will be displayed in the river.
  *

engine/tests/ElggCoreRiverAPITest.php

@@ -18,13 +18,13 @@ public function testCanCreateRiverItem() {
 
 		$id = elgg_create_river_item($params);
 		$this->assertTrue(is_int($id));
-		$this->assertTrue(_elgg_delete_river(array('id' => $id)));
+		$this->assertTrue(elgg_delete_river(array('id' => $id)));
 
 		$params['return_item'] = true;
 		$item = elgg_create_river_item($params);
 
 		$this->assertIsA($item, ElggRiveritem::class);
-		$this->assertTrue(_elgg_delete_river(array('id' => $item->id)));
+		$this->assertTrue(elgg_delete_river(array('id' => $item->id)));
 	}
 
 	public function testRiverCreationEmitsHookAndEvent() {
@@ -168,7 +168,7 @@ public function testRiverDeleteUsesPermissionHook() {
 		$this->assertTrue($item->delete());
 	}
 
-	public function testDeprecatedDeleteRiverFunctionBypassesEventsPerms() {
+	public function testDeleteRiverFunctionTriggersEventsPerms() {
 		$entity = $this->getSomeEntity();
 		$params = array(
 			'view' => 'river/relationship/friend/create',
@@ -178,22 +178,22 @@ public function testDeprecatedDeleteRiverFunctionBypassesEventsPerms() {
 		);
 		$id = elgg_create_river_item($params);
 
-		$fired = false;
-		$handler = function () use (&$fired) {
-			$fired = true;
+		$events_fired = 0;
+		$handler = function () use (&$events_fired) {
+			$events_fired++;
 		};
 
 		elgg_register_plugin_hook_handler('permissions_check:delete', 'river', $handler);
 		elgg_register_event_handler('delete:before', 'river', $handler);
 		elgg_register_event_handler('delete:after', 'river', $handler);
 
-		_elgg_delete_river(['id' => $id]);
+		elgg_delete_river(['id' => $id]);
 
 		elgg_unregister_plugin_hook_handler('permissions_check:delete', 'river', $handler);
 		elgg_unregister_event_handler('delete:before', 'river', $handler);
 		elgg_unregister_event_handler('delete:after', 'river', $handler);
 
-		$this->assertFalse($fired);
+		$this->assertEqual($events_fired, 3);
 	}
 
 	public function testElggCreateRiverItemMissingRequiredParam() {

mod/blog/actions/blog/save.php

@@ -165,9 +165,10 @@
 				$blog->save();
 			}
 		} elseif ($old_status == 'published' && $status == 'draft') {
-			_elgg_delete_river([
+			elgg_delete_river([
 				'object_guid' => $blog->guid,
 				'action_type' => 'create',
+				'limit' => false,
 			]);
 		}