Merge pull request #14561 from jeabakker/features

feat(icons): uniform storage of entity icon cropping coordinates

docs/appendix/upgrade-notes/5.x-to-6.0.rst

@@ -37,6 +37,15 @@ alias for the ``annotations`` table has been changed from ``n_table`` to ``a_tab
 If your code uses very specific clauses (select, where, order_by, etc.) you need to update your code. If you use the
 ``\Elgg\Database\QueryBuilder`` for your query parts you should be ok.
 
+Entity Icons
+------------
+
+Cropping coordinates
+~~~~~~~~~~~~~~~~~~~~
+
+The cropping coordinates of the default icon (``icon``) are now stored in a uniform way, same as those of the other icon types.
+The metadata ``x1``, ``x2``, ``y1`` and ``y2`` no longer exist. Use the new ``\ElggEntity`` function ``getIconCoordinates()``.
+
 Changes in functions
 --------------------
 

engine/classes/Elgg/Entity/CropIcon.php

@@ -71,30 +71,7 @@ protected function prepareUpload(int $entity_guid, string $input_name, string $i
 			return;
 		}
 
-		$current = [];
-		if ($icon_type === 'icon') {
-			$current = [
-				'x1' => $entity->x1,
-				'y1' => $entity->y1,
-				'x2' => $entity->x2,
-				'y2' => $entity->y2,
-			];
-		} elseif (isset($entity->{"{$icon_type}_coords"})) {
-			$current = unserialize($entity->{"{$icon_type}_coords"});
-
-			if (!is_array($current)) {
-				$current = [];
-			}
-		}
-
-		// cast to ints
-		array_walk($current, function(&$value) {
-			$value = (int) $value;
-		});
-		// remove invalid values
-		$current = array_filter($current, function($value) {
-			return $value >= 0;
-		});
+		$current = $entity->getIconCoordinates($icon_type);
 
 		$input_cropping_coords = [
 			'x1' => (int) get_input("{$input_name}_x1", get_input('x1')),

engine/classes/Elgg/EntityIconService.php

@@ -277,21 +277,10 @@ public function saveIcon(\ElggEntity $entity, \ElggFile $file, $type = 'icon', a
 		// save cropping coordinates
 		if ($type == 'icon') {
 			$entity->icontime = time();
-			if ($x1 || $y1 || $x2 || $y2) {
-				$entity->x1 = $x1;
-				$entity->y1 = $y1;
-				$entity->x2 = $x2;
-				$entity->y2 = $y2;
-			}
-		} else {
-			if ($x1 || $y1 || $x2 || $y2) {
-				$entity->{"{$type}_coords"} = serialize([
-					'x1' => $x1,
-					'y1' => $y1,
-					'x2' => $x2,
-					'y2' => $y2,
-				]);
-			}
+		}
+
+		if ($x1 || $y1 || $x2 || $y2) {
+			$entity->saveIconCoordinates($coords);
 		}
 
 		$this->events->triggerResults("entity:{$type}:saved", $entity->getType(), [
@@ -471,17 +460,7 @@ public function getIcon(\ElggEntity $entity, $size, $type = 'icon', $generate =
 			return $icon;
 		}
 
-		if ($type === 'icon') {
-			$coords = [
-				'x1' => $entity->x1,
-				'y1' => $entity->y1,
-				'x2' => $entity->x2,
-				'y2' => $entity->y2,
-			];
-		} else {
-			$coords = $entity->{"{$type}_coords"};
-			$coords = empty($coords) ? [] : unserialize($coords);
-		}
+		$coords = $entity->getIconCoordinates($type);
 
 		$this->generateIcon($entity, $master_icon, $type, $coords, $size);
 
@@ -546,14 +525,10 @@ public function deleteIcon(\ElggEntity $entity, string $type = 'icon', bool $ret
 
 		if ($type == 'icon') {
 			unset($entity->icontime);
-			unset($entity->x1);
-			unset($entity->y1);
-			unset($entity->x2);
-			unset($entity->y2);
-		} else {
-			unset($entity->{"{$type}_coords"});
 		}
 
+		$entity->removeIconCoordinates($type);
+
 		return $result;
 	}
 

engine/classes/Elgg/Traits/Entity/Icons.php

@@ -2,6 +2,9 @@
 
 namespace Elgg\Traits\Entity;
 
+use Elgg\Exceptions\InvalidArgumentException;
+use Elgg\Exceptions\RangeException;
+
 /**
  * Adds helper functions to \ElggEntity in relation to icons.
  *
@@ -111,4 +114,66 @@ public function hasIcon(string $size, string $type = 'icon'): bool {
 	public function getIconURL(string|array $params = []): string {
 		return _elgg_services()->iconService->getIconURL($this, $params);
 	}
+
+	/**
+	 * Save cropping coordinates for an icon type
+	 *
+	 * @param array  $coords An array of cropping coordinates x1, y1, x2, y2
+	 * @param string $type   The name of the icon. e.g., 'icon', 'cover_photo'
+	 *
+	 * @return void
+	 */
+	public function saveIconCoordinates(array $coords, string $type = 'icon'): void {
+		// remove noise from the coords array
+		$allowed_keys = ['x1', 'x2', 'y1', 'y2'];
+		$coords = array_filter($coords, function($value, $key) use ($allowed_keys) {
+			return in_array($key, $allowed_keys) && is_int($value);
+		}, ARRAY_FILTER_USE_BOTH);
+
+		if (!isset($coords['x1']) || !isset($coords['x2']) || !isset($coords['y1']) || !isset($coords['y2'])) {
+			throw new InvalidArgumentException('Please provide correct coordinates [x1, x2, y1, y2]');
+		}
+
+		if ($coords['x1'] < 0 || $coords['x2'] < 0 || $coords['y1'] < 0 || $coords['y2'] < 0) {
+			throw new RangeException("Coordinates can't have negative numbers");
+		}
+
+		$this->{"{$type}_coords"} = serialize($coords);
+	}
+
+	/**
+	 * Get the cropping coordinates for an icon type
+	 *
+	 * @param string $type The name of the icon. e.g., 'icon', 'cover_photo'
+	 *
+	 * @return null|array
+	 */
+	public function getIconCoordinates(string $type = 'icon'): array {
+		if (!isset($this->{"{$type}_coords"})) {
+			return [];
+		}
+
+		$coords = unserialize($this->{"{$type}_coords"}) ?: [];
+
+		// cast to integers
+		array_walk($coords, function(&$value) {
+			$value = (int) $value;
+		});
+
+		// remove invalid values
+		return array_filter($coords, function($value) {
+			return $value >= 0;
+		});
+	}
+
+	/**
+	 * Remove the cropping coordinates of an icon type
+	 *
+	 * @param string $type The name of the icon. e.g., 'icon', 'cover_photo'
+	 *
+	 * @return void
+	 */
+	public function removeIconCoordinates(string $type = 'icon'): void {
+		unset($this->{"{$type}_coords"});
+	}
 }

engine/classes/Elgg/Upgrades/MigrateEntityIconCroppingCoordinates.php

@@ -0,0 +1,94 @@
+<?php
+
+namespace Elgg\Upgrades;
+
+use Elgg\Upgrade\Result;
+
+class MigrateEntityIconCroppingCoordinates extends \Elgg\Upgrade\SystemUpgrade {
+
+	/**
+	 * {@inheritdoc}
+	 */
+	public function getVersion(): int {
+		return 2024020101;
+	}
+
+	/**
+	 * {@inheritdoc}
+	 */
+	public function shouldBeSkipped(): bool {
+		return empty($this->countItems());
+	}
+
+	/**
+	 * {@inheritdoc}
+	 */
+	public function needsIncrementOffset(): bool {
+		return false;
+	}
+
+	/**
+	 * {@inheritdoc}
+	 */
+	public function countItems(): int {
+		return elgg_count_entities($this->getOptions());
+	}
+
+	/**
+	 * {@inheritdoc}
+	 */
+	public function run(Result $result, $offset): Result {
+		/* @var $entities \ElggBatch */
+		$entities = elgg_get_entities($this->getOptions([
+			'offset' => $offset,
+		]));
+		/* @var $entity \ElggEntity */
+		foreach ($entities as $entity) {
+			$coords = [
+				'x1' => (int) $entity->x1,
+				'x2' => (int) $entity->x2,
+				'y1' => (int) $entity->y1,
+				'y2' => (int) $entity->y2,
+			];
+
+			try {
+				$entity->saveIconCoordinates($coords, 'icon');
+			} catch (\Elgg\Exceptions\ExceptionInterface $e) {
+				// something went wrong with the coords, probably broken
+			}
+
+			unset($entity->x1);
+			unset($entity->x2);
+			unset($entity->y1);
+			unset($entity->y2);
+
+			$result->addSuccesses();
+		}
+
+		return $result;
+	}
+
+	/**
+	 * Get options for the upgrade
+	 *
+	 * @param array $options additional options
+	 *
+	 * @return array
+	 * @see elgg_get_entities()
+	 */
+	protected function getOptions(array $options = []): array {
+		$defaults = [
+			'limit' => 50,
+			'batch' => true,
+			'batch_inc_offset' => $this->needsIncrementOffset(),
+			'metadata_names' => [
+				'x1',
+				'x2',
+				'y1',
+				'y2',
+			],
+		];
+
+		return array_merge($defaults, $options);
+	}
+}