From 0df2e5b471db341c211f72ee3cab068c82d1f48a Mon Sep 17 00:00:00 2001 From: paul121 Date: Tue, 26 Oct 2021 16:52:13 -0700 Subject: [PATCH 1/4] Query group members from multiple groups in a single query. --- docs/development/module/services.md | 2 +- .../EventSubscriber/LogEventSubscriber.php | 4 +- .../asset/group/src/GroupAssetLocation.php | 17 ++----- modules/asset/group/src/GroupMembership.php | 27 +++++++---- .../group/src/GroupMembershipInterface.php | 10 ++-- .../src/Plugin/views/argument/AssetGroup.php | 2 +- .../group/tests/src/Kernel/GroupTest.php | 46 +++++++++++++++---- 7 files changed, 67 insertions(+), 41 deletions(-) diff --git a/docs/development/module/services.md b/docs/development/module/services.md index c94c6f5e6..79cc18d59 100644 --- a/docs/development/module/services.md +++ b/docs/development/module/services.md @@ -97,7 +97,7 @@ array of asset entities. `getGroupAssignmentLog($asset)` - Find the latest group assignment log that references an asset. Returns a log entity, or `NULL` if no logs were found. -`getGroupMembers($group, $recurse)` - Get assets that are members of a group, +`getGroupMembers($groups, $recurse)` - Get assets that are members of groups, optionally recursing into child groups. **Example usage:** diff --git a/modules/asset/group/src/EventSubscriber/LogEventSubscriber.php b/modules/asset/group/src/EventSubscriber/LogEventSubscriber.php index 77b941626..ce140b1ea 100644 --- a/modules/asset/group/src/EventSubscriber/LogEventSubscriber.php +++ b/modules/asset/group/src/EventSubscriber/LogEventSubscriber.php @@ -175,7 +175,7 @@ class LogEventSubscriber implements EventSubscriberInterface { if ($asset->bundle() === 'group') { $member_tags = array_map(function (AssetInterface $asset) { return $asset->getCacheTags(); - }, $this->groupMembership->getGroupMembers($asset)); + }, $this->groupMembership->getGroupMembers([$asset])); array_push($tags, ...array_merge(...$member_tags)); } } @@ -188,7 +188,7 @@ class LogEventSubscriber implements EventSubscriberInterface { if ($asset->bundle() === 'group') { $member_tags = array_map(function (AssetInterface $asset) { return $asset->getCacheTags(); - }, $this->groupMembership->getGroupMembers($asset)); + }, $this->groupMembership->getGroupMembers([$asset])); array_push($tags, ...array_merge(...$member_tags)); } } diff --git a/modules/asset/group/src/GroupAssetLocation.php b/modules/asset/group/src/GroupAssetLocation.php index 3a0799179..a6d4e82bb 100644 --- a/modules/asset/group/src/GroupAssetLocation.php +++ b/modules/asset/group/src/GroupAssetLocation.php @@ -116,20 +116,11 @@ class GroupAssetLocation extends AssetLocation implements AssetLocationInterface /** @var \Drupal\asset\Entity\AssetInterface[] $assets */ $assets = parent::getAssetsByLocation($location); - // Iterate through the assets to check if any of them are groups. - $groups = []; - foreach ($assets as $asset) { - if ($asset->bundle() == 'group') { - $groups[] = $asset; - } - } - // Recursively load all group members and add them to the list of assets. - /** @var \Drupal\asset\Entity\AssetInterface[] $members */ - $members = []; - foreach ($groups as $group) { - $members = array_merge($members, $this->groupMembership->getGroupMembers($group, TRUE)); - } + $groups = array_filter($assets, function (AssetInterface $asset) { + return $asset->bundle() === 'group'; + }); + $members = $this->groupMembership->getGroupMembers($groups, TRUE); $assets = array_merge($assets, $members); // It is possible for a group member asset to be in a different location diff --git a/modules/asset/group/src/GroupMembership.php b/modules/asset/group/src/GroupMembership.php index c5740c513..f58fb5d2b 100644 --- a/modules/asset/group/src/GroupMembership.php +++ b/modules/asset/group/src/GroupMembership.php @@ -147,10 +147,19 @@ class GroupMembership implements GroupMembershipInterface { /** * {@inheritdoc} */ - public function getGroupMembers(AssetInterface $group, bool $recurse = TRUE): array { - if (empty($group->id())) { + public function getGroupMembers(array $groups, bool $recurse = TRUE): array { + + // Get group ids. + $group_ids = array_map(function (AssetInterface $group) { + return $group->id(); + }, $groups); + + // Bail if there are no group ids. + if (empty($group_ids)) { return []; } + + // Build query for group members. $query = " -- Select asset IDs from the asset base table. SELECT a.id @@ -181,13 +190,13 @@ class GroupMembership implements GroupMembershipInterface { -- Limit results to completed membership assignment logs to the desired -- group that took place before the given timestamp. - WHERE (lfd.is_group_assignment = 1) AND (lfd.status = 'done') AND (lfd.timestamp <= :timestamp) AND (lg.group_target_id = :group_id) + WHERE (lfd.is_group_assignment = 1) AND (lfd.status = 'done') AND (lfd.timestamp <= :timestamp) AND (lg.group_target_id IN (:group_ids[])) -- Exclude records with future log entries. AND lfd2.id IS NULL"; $args = [ ':timestamp' => $this->time->getRequestTime(), - ':group_id' => $group->id(), + ':group_ids[]' => $group_ids, ]; $result = $this->database->query($query, $args)->fetchAll(); $asset_ids = []; @@ -203,11 +212,11 @@ class GroupMembership implements GroupMembershipInterface { /** @var \Drupal\asset\Entity\AssetInterface[] $assets */ $assets = $this->entityTypeManager->getStorage('asset')->loadMultiple($asset_ids); if ($recurse) { - foreach ($assets as $asset) { - if ($asset->bundle() == 'group') { - $assets = array_merge($assets, $this->getGroupMembers($asset)); - } - } + // Iterate through the assets to check if any of them are groups. + $groups = array_filter($assets, function (AssetInterface $asset) { + return $asset->bundle() === 'group'; + }); + $assets = array_merge($assets, $this->getGroupMembers($groups)); } return $assets; } diff --git a/modules/asset/group/src/GroupMembershipInterface.php b/modules/asset/group/src/GroupMembershipInterface.php index 4630183d5..590239f9b 100644 --- a/modules/asset/group/src/GroupMembershipInterface.php +++ b/modules/asset/group/src/GroupMembershipInterface.php @@ -44,17 +44,17 @@ interface GroupMembershipInterface { public function getGroupAssignmentLog(AssetInterface $asset): ?LogInterface; /** - * Get assets that are members of a group. + * Get assets that are members of groups. * - * @param \Drupal\asset\Entity\AssetInterface $group - * The Asset entity. + * @param \Drupal\asset\Entity\AssetInterface[] $groups + * An array of group assets to lookup. * @param bool $recurse * Boolean: whether or not to recurse and load members of sub-groups. * Defaults to TRUE. * - * @return array + * @return \Drupal\asset\Entity\AssetInterface[] * Returns an array of assets. */ - public function getGroupMembers(AssetInterface $group, bool $recurse = TRUE): array; + public function getGroupMembers(array $groups, bool $recurse = TRUE): array; } diff --git a/modules/asset/group/src/Plugin/views/argument/AssetGroup.php b/modules/asset/group/src/Plugin/views/argument/AssetGroup.php index 5f21fd6d9..de5715601 100644 --- a/modules/asset/group/src/Plugin/views/argument/AssetGroup.php +++ b/modules/asset/group/src/Plugin/views/argument/AssetGroup.php @@ -32,7 +32,7 @@ class AssetGroup extends ArgumentPluginBase { // simple. It only needs the condition: "WHERE asset.id IN (:asset_ids)". // See https://www.drupal.org/project/farm/issues/3217184 for more info. $group = \Drupal::entityTypeManager()->getStorage('asset')->load($this->argument); - $assets = \Drupal::service('group.membership')->getGroupMembers($group); + $assets = \Drupal::service('group.membership')->getGroupMembers([$group]); $asset_ids = []; foreach ($assets as $asset) { $asset_ids[] = $asset->id(); diff --git a/modules/asset/group/tests/src/Kernel/GroupTest.php b/modules/asset/group/tests/src/Kernel/GroupTest.php index c9857299e..c42af4c15 100644 --- a/modules/asset/group/tests/src/Kernel/GroupTest.php +++ b/modules/asset/group/tests/src/Kernel/GroupTest.php @@ -106,8 +106,8 @@ class GroupTest extends KernelTestBase { // When an asset has no group assignment logs, it has no group membership. $this->assertFalse($this->groupMembership->hasGroup($animal), 'New assets do not have group membership.'); $this->assertEmpty($this->groupMembership->getGroup($animal), 'New assets do not reference any groups.'); - $this->assertEmpty($this->groupMembership->getGroupMembers($first_group), 'New groups have no members.'); - $this->assertEmpty($this->groupMembership->getGroupMembers($second_group), 'New groups have no members.'); + $this->assertEmpty($this->groupMembership->getGroupMembers([$first_group]), 'New groups have no members.'); + $this->assertEmpty($this->groupMembership->getGroupMembers([$second_group]), 'New groups have no members.'); // Assert that the animal's cache tags were not invalidated. $this->assertEntityTestCache($animal, TRUE); @@ -126,8 +126,8 @@ class GroupTest extends KernelTestBase { // When an asset has a done group assignment logs, it has group membership. $this->assertTrue($this->groupMembership->hasGroup($animal), 'Asset with group assignment has group membership.'); $this->assertEquals($first_group->id(), $this->groupMembership->getGroup($animal)[0]->id(), 'Asset with group assignment is in the assigned group.'); - $this->assertEquals(1, count($this->groupMembership->getGroupMembers($first_group)), 'When an asset becomes a group member, the group has one member.'); - $this->assertEmpty($this->groupMembership->getGroupMembers($second_group), 'When an asset becomes a group member, other groups are unaffected.'); + $this->assertEquals(1, count($this->groupMembership->getGroupMembers([$first_group])), 'When an asset becomes a group member, the group has one member.'); + $this->assertEmpty($this->groupMembership->getGroupMembers([$second_group]), 'When an asset becomes a group member, other groups are unaffected.'); // Assert that the animal's cache tags were invalidated. $this->assertEntityTestCache($animal, FALSE); @@ -149,7 +149,7 @@ class GroupTest extends KernelTestBase { // When an asset has a pending group assignment logs, it still has the same // group membership as before. $this->assertEquals($first_group->id(), $this->groupMembership->getGroup($animal)[0]->id(), 'Pending group assignment logs do not affect membership.'); - $this->assertEmpty($this->groupMembership->getGroupMembers($second_group), 'Groups with only pending membership have zero members.'); + $this->assertEmpty($this->groupMembership->getGroupMembers([$second_group]), 'Groups with only pending membership have zero members.'); // Assert that the animal's cache tags were not invalidated. $this->assertEntityTestCache($animal, TRUE); @@ -158,7 +158,7 @@ class GroupTest extends KernelTestBase { $second_log->status = 'done'; $second_log->save(); $this->assertEquals($second_group->id(), $this->groupMembership->getGroup($animal)[0]->id(), 'A second group assignment log updates group membership.'); - $this->assertEquals(1, count($this->groupMembership->getGroupMembers($second_group)), 'Completed group assignment logs add group members.'); + $this->assertEquals(1, count($this->groupMembership->getGroupMembers([$second_group])), 'Completed group assignment logs add group members.'); // Assert that the animal's cache tags were invalidated. $this->assertEntityTestCache($animal, FALSE); @@ -181,7 +181,7 @@ class GroupTest extends KernelTestBase { // When an asset has a "done" group assignment log in the future, the asset // group membership remains the same as the previous "done" movement log. $this->assertEquals($second_group->id(), $this->groupMembership->getGroup($animal)[0]->id(), 'A third group assignment log in the future does not update group membership.'); - $this->assertEquals(1, count($this->groupMembership->getGroupMembers($second_group)), 'Future group assignment logs do not affect members.'); + $this->assertEquals(1, count($this->groupMembership->getGroupMembers([$second_group])), 'Future group assignment logs do not affect members.'); // Assert that the animal's cache tags were not invalidated. $this->assertEntityTestCache($animal, TRUE); @@ -201,8 +201,8 @@ class GroupTest extends KernelTestBase { // effectively "unsets" the asset's group membership. $this->assertFalse($this->groupMembership->hasGroup($animal), 'Asset group membership can be unset.'); $this->assertEmpty($this->groupMembership->getGroup($animal), 'Unset group membership does not reference any groups.'); - $this->assertEquals(0, count($this->groupMembership->getGroupMembers($first_group)), 'Unset group membership unsets group members.'); - $this->assertEquals(0, count($this->groupMembership->getGroupMembers($second_group)), 'Unset group membership unsets group members.'); + $this->assertEquals(0, count($this->groupMembership->getGroupMembers([$first_group])), 'Unset group membership unsets group members.'); + $this->assertEquals(0, count($this->groupMembership->getGroupMembers([$second_group])), 'Unset group membership unsets group members.'); // Assert that the animal's cache tags were invalidated. $this->assertEntityTestCache($animal, FALSE); @@ -215,10 +215,36 @@ class GroupTest extends KernelTestBase { // When a group membership is deleted the last group membership log is used. $this->assertEquals($second_group->id(), $this->groupMembership->getGroup($animal)[0]->id(), 'Deleting a group membership log updates group membership.'); - $this->assertEquals(1, count($this->groupMembership->getGroupMembers($second_group)), 'Deleting a group membership log updates group members.'); + $this->assertEquals(1, count($this->groupMembership->getGroupMembers([$second_group])), 'Deleting a group membership log updates group members.'); // Assert that the animal's cache tags were invalidated. $this->assertEntityTestCache($animal, FALSE); + + // Create a second animal. + /** @var \Drupal\asset\Entity\AssetInterface $second_animal */ + $second_animal = Asset::create([ + 'type' => 'animal', + 'name' => $this->randomMachineName(), + 'status' => 'active', + ]); + $second_animal->save(); + + // Create a "done" log that assigns the second animal to the first group. + /** @var \Drupal\log\Entity\LogInterface $fifth_log */ + $fifth_log = Log::create([ + 'type' => 'test', + 'status' => 'done', + 'is_group_assignment' => TRUE, + 'group' => ['target_id' => $first_group->id()], + 'asset' => ['target_id' => $second_animal->id()], + ]); + $fifth_log->save(); + + // Assert that group members from multiple groups can be queried together. + $this->assertEquals($second_group->id(), $this->groupMembership->getGroup($animal)[0]->id(), 'The first animal is in the second group.'); + $this->assertEquals($first_group->id(), $this->groupMembership->getGroup($second_animal)[0]->id(), 'The second animal is in the first group.'); + $group_members = $this->groupMembership->getGroupMembers([$first_group, $second_group]); + $this->assertEquals(2, count($group_members), 'Group members from multiple groups can be queried together.'); } /** From 937001a13a89c09cacb1ceda3a4f036dc6ffb19b Mon Sep 17 00:00:00 2001 From: paul121 Date: Tue, 26 Oct 2021 17:40:18 -0700 Subject: [PATCH 2/4] Optimize group member logic in the group log event subscriber. --- .../EventSubscriber/LogEventSubscriber.php | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/modules/asset/group/src/EventSubscriber/LogEventSubscriber.php b/modules/asset/group/src/EventSubscriber/LogEventSubscriber.php index ce140b1ea..c30bf193c 100644 --- a/modules/asset/group/src/EventSubscriber/LogEventSubscriber.php +++ b/modules/asset/group/src/EventSubscriber/LogEventSubscriber.php @@ -167,31 +167,31 @@ class LogEventSubscriber implements EventSubscriberInterface { // @todo Only invalidate cache if the movement log changes the group's current location. This might be different for each asset. $tags = []; - // Include assets that were previously referenced. + // Include group assets that were previously referenced. if (!empty($log->original)) { - foreach ($log->original->get('asset')->referencedEntities() as $asset) { - // If the asset is a group asset, collect group member cache tags. - if ($asset->bundle() === 'group') { - $member_tags = array_map(function (AssetInterface $asset) { - return $asset->getCacheTags(); - }, $this->groupMembership->getGroupMembers([$asset])); - array_push($tags, ...array_merge(...$member_tags)); - } - } + // Get all group assets. + $group_assets = array_filter($log->original->get('asset')->referencedEntities(), function (AssetInterface $asset) { + return $asset->bundle() === 'group'; + }); + + // Collect group member cache tags. + $member_tags = array_map(function (AssetInterface $asset) { + return $asset->getCacheTags(); + }, $this->groupMembership->getGroupMembers($group_assets)); + array_push($tags, ...array_merge(...$member_tags)); } - // Include assets currently referenced by the log. - foreach ($log->get('asset')->referencedEntities() as $asset) { + // Include group assets currently referenced by the log. + $group_assets = array_filter($log->get('asset')->referencedEntities(), function (AssetInterface $asset) { + return $asset->bundle() === 'group'; + }); - // If the asset is a group asset, collect group member cache tags. - if ($asset->bundle() === 'group') { - $member_tags = array_map(function (AssetInterface $asset) { - return $asset->getCacheTags(); - }, $this->groupMembership->getGroupMembers([$asset])); - array_push($tags, ...array_merge(...$member_tags)); - } - } + // Collect group member cache tags. + $member_tags = array_map(function (AssetInterface $asset) { + return $asset->getCacheTags(); + }, $this->groupMembership->getGroupMembers($group_assets)); + array_push($tags, ...array_merge(...$member_tags)); // Invalidate the cache tags. $this->cacheTagsInvalidator->invalidateTags($tags); From 50d7f643c2a837c10c7d8cf829c4d725c03b9011 Mon Sep 17 00:00:00 2001 From: paul121 Date: Tue, 26 Oct 2021 17:18:04 -0700 Subject: [PATCH 3/4] Use dependency injection in views argument plugins. --- .../src/Plugin/views/argument/AssetGroup.php | 54 +++++++++++++++++- .../Plugin/views/argument/AssetLocation.php | 55 ++++++++++++++++++- 2 files changed, 105 insertions(+), 4 deletions(-) diff --git a/modules/asset/group/src/Plugin/views/argument/AssetGroup.php b/modules/asset/group/src/Plugin/views/argument/AssetGroup.php index de5715601..9fcfe24db 100644 --- a/modules/asset/group/src/Plugin/views/argument/AssetGroup.php +++ b/modules/asset/group/src/Plugin/views/argument/AssetGroup.php @@ -2,7 +2,10 @@ namespace Drupal\farm_group\Plugin\views\argument; +use Drupal\Core\Entity\EntityTypeManagerInterface; +use Drupal\farm_group\GroupMembershipInterface; use Drupal\views\Plugin\views\argument\ArgumentPluginBase; +use Symfony\Component\DependencyInjection\ContainerInterface; /** * An argument for filtering assets by their current group. @@ -13,6 +16,53 @@ use Drupal\views\Plugin\views\argument\ArgumentPluginBase; */ class AssetGroup extends ArgumentPluginBase { + /** + * The entity type manager service. + * + * @var \Drupal\Core\Entity\EntityTypeManagerInterface + */ + protected $entityTypeManager; + + /** + * The group membership service. + * + * @var \Drupal\farm_group\GroupMembershipInterface + */ + protected $groupMembership; + + /** + * Constructs an AssetGroup object. + * + * @param array $configuration + * A configuration array containing information about the plugin instance. + * @param string $plugin_id + * The plugin_id for the plugin instance. + * @param mixed $plugin_definition + * The plugin implementation definition. + * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager + * The entity type manager service. + * @param \Drupal\farm_group\GroupMembershipInterface $group_membership + * The group membership service. + */ + public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, GroupMembershipInterface $group_membership) { + parent::__construct($configuration, $plugin_id, $plugin_definition); + $this->entityTypeManager = $entity_type_manager; + $this->groupMembership = $group_membership; + } + + /** + * {@inheritdoc} + */ + public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) { + return new static( + $configuration, + $plugin_id, + $plugin_definition, + $container->get('entity_type.manager'), + $container->get('group.membership'), + ); + } + /** * {@inheritdoc} * @@ -31,8 +81,8 @@ class AssetGroup extends ArgumentPluginBase { // 3. It keeps this Views argument handler's query modifications very // simple. It only needs the condition: "WHERE asset.id IN (:asset_ids)". // See https://www.drupal.org/project/farm/issues/3217184 for more info. - $group = \Drupal::entityTypeManager()->getStorage('asset')->load($this->argument); - $assets = \Drupal::service('group.membership')->getGroupMembers([$group]); + $group = $this->entityTypeManager->getStorage('asset')->load($this->argument); + $assets = $this->groupMembership->getGroupMembers([$group]); $asset_ids = []; foreach ($assets as $asset) { $asset_ids[] = $asset->id(); diff --git a/modules/core/location/src/Plugin/views/argument/AssetLocation.php b/modules/core/location/src/Plugin/views/argument/AssetLocation.php index a7c0ebfb3..ccb916271 100644 --- a/modules/core/location/src/Plugin/views/argument/AssetLocation.php +++ b/modules/core/location/src/Plugin/views/argument/AssetLocation.php @@ -2,7 +2,10 @@ namespace Drupal\farm_location\Plugin\views\argument; +use Drupal\Core\Entity\EntityTypeManagerInterface; +use Drupal\farm_location\AssetLocationInterface; use Drupal\views\Plugin\views\argument\ArgumentPluginBase; +use Symfony\Component\DependencyInjection\ContainerInterface; /** * An argument for filtering assets by their current location. @@ -13,6 +16,53 @@ use Drupal\views\Plugin\views\argument\ArgumentPluginBase; */ class AssetLocation extends ArgumentPluginBase { + /** + * The entity type manager service. + * + * @var \Drupal\Core\Entity\EntityTypeManagerInterface + */ + protected $entityTypeManager; + + /** + * The asset location service. + * + * @var \Drupal\farm_location\AssetLocationInterface + */ + protected $assetLocation; + + /** + * Constructs an AssetLocation object. + * + * @param array $configuration + * A configuration array containing information about the plugin instance. + * @param string $plugin_id + * The plugin_id for the plugin instance. + * @param mixed $plugin_definition + * The plugin implementation definition. + * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager + * The entity type manager service. + * @param \Drupal\farm_location\AssetLocationInterface $asset_location + * The asset location service. + */ + public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, AssetLocationInterface $asset_location) { + parent::__construct($configuration, $plugin_id, $plugin_definition); + $this->entityTypeManager = $entity_type_manager; + $this->assetLocation = $asset_location; + } + + /** + * {@inheritdoc} + */ + public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) { + return new static( + $configuration, + $plugin_id, + $plugin_definition, + $container->get('entity_type.manager'), + $container->get('asset.location'), + ); + } + /** * {@inheritdoc} */ @@ -29,8 +79,9 @@ class AssetLocation extends ArgumentPluginBase { // 3. It keeps this Views argument handler's query modifications very // simple. It only needs the condition: "WHERE asset.id IN (:asset_ids)". // See https://www.drupal.org/project/farm/issues/3217168 for more info. - $location = \Drupal::entityTypeManager()->getStorage('asset')->load($this->argument); - $assets = \Drupal::service('asset.location')->getAssetsByLocation($location); + /** @var \Drupal\asset\Entity\AssetInterface $location */ + $location = $this->entityTypeManager->getStorage('asset')->load($this->argument); + $assets = $this->assetLocation->getAssetsByLocation($location); $asset_ids = []; foreach ($assets as $asset) { $asset_ids[] = $asset->id(); From 9937730c1413e5d82b942d2ac04f25d06ffda844 Mon Sep 17 00:00:00 2001 From: paul121 Date: Wed, 27 Oct 2021 09:03:53 -0700 Subject: [PATCH 4/4] Query assets from multiple locations in a single query. --- docs/development/module/services.md | 2 +- .../asset/group/src/GroupAssetLocation.php | 35 +++++++++++-------- .../group/tests/src/Kernel/GroupTest.php | 22 ++++++------ modules/core/location/src/AssetLocation.php | 17 ++++++--- .../location/src/AssetLocationInterface.php | 10 +++--- .../Plugin/views/argument/AssetLocation.php | 2 +- .../tests/src/Kernel/LocationTest.php | 28 ++++++++------- 7 files changed, 67 insertions(+), 49 deletions(-) diff --git a/docs/development/module/services.md b/docs/development/module/services.md index 79cc18d59..58690db6a 100644 --- a/docs/development/module/services.md +++ b/docs/development/module/services.md @@ -38,7 +38,7 @@ asset. Returns a log entity, or `NULL` if no logs were found. `setIntrinsicGeometry($asset, $wkt)` - Set an asset's intrinsic geometry, given a string in Well-Known Text format. -`getAssetsByLocation($location)` - Get assets that are in a location. +`getAssetsByLocation($locations)` - Get assets that are in locations. **Example usage**: diff --git a/modules/asset/group/src/GroupAssetLocation.php b/modules/asset/group/src/GroupAssetLocation.php index a6d4e82bb..9f5343917 100644 --- a/modules/asset/group/src/GroupAssetLocation.php +++ b/modules/asset/group/src/GroupAssetLocation.php @@ -110,11 +110,11 @@ class GroupAssetLocation extends AssetLocation implements AssetLocationInterface /** * {@inheritdoc} */ - public function getAssetsByLocation(AssetInterface $location): array { + public function getAssetsByLocation(array $locations): array { // First delegate to the parent function to get assets in the location. /** @var \Drupal\asset\Entity\AssetInterface[] $assets */ - $assets = parent::getAssetsByLocation($location); + $assets = parent::getAssetsByLocation($locations); // Recursively load all group members and add them to the list of assets. $groups = array_filter($assets, function (AssetInterface $asset) { @@ -123,22 +123,27 @@ class GroupAssetLocation extends AssetLocation implements AssetLocationInterface $members = $this->groupMembership->getGroupMembers($groups, TRUE); $assets = array_merge($assets, $members); + // Get location ids. + $location_ids = array_map(function (AssetInterface $location) { + return $location->id(); + }, $locations); + // It is possible for a group member asset to be in a different location // than the group, if it has a movement log that is more recent than the // group's. So iterate through all the assets and remove any that are not in - // the location. The asset may be in multiple locations (including this - // one), so we only want to remove it if none of its locations match. - foreach ($assets as $key => $asset) { - $match = FALSE; - foreach ($this->getLocation($asset) as $asset_location) { - if ($asset_location->id() == $location->id()) { - $match = TRUE; - } - } - if (!$match) { - unset($assets[$key]); - } - } + // one of the specified locations. The asset may be in multiple locations + // (including this one), so we only want to remove it if none of its + // locations match. + $assets = array_filter($assets, function (AssetInterface $asset) use ($location_ids) { + + // Get asset location ids. + $asset_location_ids = array_map(function (AssetInterface $location) { + return $location->id(); + }, $this->getLocation($asset)); + + // Only include the asset if it is in one of the specified locations. + return !empty(array_intersect($location_ids, $asset_location_ids)); + }); // Return the assets. return $assets; diff --git a/modules/asset/group/tests/src/Kernel/GroupTest.php b/modules/asset/group/tests/src/Kernel/GroupTest.php index c42af4c15..b5dc20c55 100644 --- a/modules/asset/group/tests/src/Kernel/GroupTest.php +++ b/modules/asset/group/tests/src/Kernel/GroupTest.php @@ -307,7 +307,7 @@ class GroupTest extends KernelTestBase { $second_pasture->save(); // Confirm that new locations are empty. - $this->assertEmpty($this->assetLocation->getAssetsByLocation($first_pasture), 'New locations are empty.'); + $this->assertEmpty($this->assetLocation->getAssetsByLocation([$first_pasture]), 'New locations are empty.'); // Create a log that moves the animal to the first pasture. /** @var \Drupal\log\Entity\LogInterface $second_log */ @@ -323,8 +323,8 @@ class GroupTest extends KernelTestBase { // Confirm that the animal is located in the first pasture. $this->assertEquals($this->logLocation->getLocation($second_log), $this->assetLocation->getLocation($animal), 'Asset location is determined by asset membership log.'); $this->assertEquals($this->logLocation->getGeometry($second_log), $this->assetLocation->getGeometry($animal), 'Asset geometry is determined by asset membership log.'); - $this->assertEquals(1, count($this->assetLocation->getAssetsByLocation($first_pasture)), 'Locations have assets that are moved to them.'); - $this->assertEmpty($this->assetLocation->getAssetsByLocation($second_pasture), 'Locations that do not have assets moved to them are unaffected.'); + $this->assertEquals(1, count($this->assetLocation->getAssetsByLocation([$first_pasture])), 'Locations have assets that are moved to them.'); + $this->assertEmpty($this->assetLocation->getAssetsByLocation([$second_pasture]), 'Locations that do not have assets moved to them are unaffected.'); // Assert that the animal's cache tags were invalidated. $this->assertEntityTestCache($animal, FALSE); @@ -346,8 +346,8 @@ class GroupTest extends KernelTestBase { // Confirm that the animal is located in the second pasture. $this->assertEquals($this->logLocation->getLocation($third_log), $this->assetLocation->getLocation($animal), 'Asset location is determined by group membership log.'); $this->assertEquals($this->logLocation->getGeometry($third_log), $this->assetLocation->getGeometry($animal), 'Asset geometry is determined by group membership log.'); - $this->assertEmpty($this->assetLocation->getAssetsByLocation($first_pasture), 'A group movement removes assets from their previous location.'); - $this->assertEquals(2, count($this->assetLocation->getAssetsByLocation($second_pasture)), 'A group movement adds assets to their new location.'); + $this->assertEmpty($this->assetLocation->getAssetsByLocation([$first_pasture]), 'A group movement removes assets from their previous location.'); + $this->assertEquals(2, count($this->assetLocation->getAssetsByLocation([$second_pasture])), 'A group movement adds assets to their new location.'); // Assert that the animal's cache tags were invalidated. $this->assertEntityTestCache($animal, FALSE); @@ -369,8 +369,8 @@ class GroupTest extends KernelTestBase { // Confirm that the animal location was unset. $this->assertEquals($this->logLocation->getLocation($fourth_log), $this->assetLocation->getLocation($animal), 'Asset location can be unset by group membership log.'); $this->assertEquals($this->logLocation->getGeometry($fourth_log), $this->assetLocation->getGeometry($animal), 'Asset geometry can be unset by group membership log.'); - $this->assertEmpty($this->assetLocation->getAssetsByLocation($first_pasture), 'Unsetting group location removes member assets from all locations.'); - $this->assertEmpty($this->assetLocation->getAssetsByLocation($second_pasture), 'Unsetting group location removes member assets from all locations.'); + $this->assertEmpty($this->assetLocation->getAssetsByLocation([$first_pasture]), 'Unsetting group location removes member assets from all locations.'); + $this->assertEmpty($this->assetLocation->getAssetsByLocation([$second_pasture]), 'Unsetting group location removes member assets from all locations.'); // Assert that the animal's cache tags were invalidated. $this->assertEntityTestCache($animal, FALSE); @@ -393,8 +393,8 @@ class GroupTest extends KernelTestBase { // logs now. $this->assertEquals($this->logLocation->getLocation($second_log), $this->assetLocation->getLocation($animal), 'Asset location is determined by asset membership log.'); $this->assertEquals($this->logLocation->getGeometry($second_log), $this->assetLocation->getGeometry($animal), 'Asset geometry is determined by asset membership log.'); - $this->assertEquals(1, count($this->assetLocation->getAssetsByLocation($first_pasture)), 'Unsetting group membership adds assets to their previous location.'); - $this->assertEmpty($this->assetLocation->getAssetsByLocation($second_pasture), 'Unsetting group membership removes member assets from the group location.'); + $this->assertEquals(1, count($this->assetLocation->getAssetsByLocation([$first_pasture])), 'Unsetting group membership adds assets to their previous location.'); + $this->assertEmpty($this->assetLocation->getAssetsByLocation([$second_pasture]), 'Unsetting group membership removes member assets from the group location.'); // Assert that the animal's cache tags were invalidated. $this->assertEntityTestCache($animal, FALSE); @@ -412,8 +412,8 @@ class GroupTest extends KernelTestBase { // Confirm that the animal is located in the second pasture. $this->assertEquals($this->logLocation->getLocation($third_log), $this->assetLocation->getLocation($animal), 'Asset location is determined by group membership log.'); $this->assertEquals($this->logLocation->getGeometry($third_log), $this->assetLocation->getGeometry($animal), 'Asset geometry is determined by group membership log.'); - $this->assertEmpty($this->assetLocation->getAssetsByLocation($first_pasture), 'A group movement removes assets from their previous location.'); - $this->assertEquals(2, count($this->assetLocation->getAssetsByLocation($second_pasture)), 'A group movement adds assets to their new location.'); + $this->assertEmpty($this->assetLocation->getAssetsByLocation([$first_pasture]), 'A group movement removes assets from their previous location.'); + $this->assertEquals(2, count($this->assetLocation->getAssetsByLocation([$second_pasture])), 'A group movement adds assets to their new location.'); // Assert that the animal's cache tags were invalidated. $this->assertEntityTestCache($animal, FALSE); diff --git a/modules/core/location/src/AssetLocation.php b/modules/core/location/src/AssetLocation.php index d25bfa69b..460a38cde 100644 --- a/modules/core/location/src/AssetLocation.php +++ b/modules/core/location/src/AssetLocation.php @@ -227,10 +227,19 @@ class AssetLocation implements AssetLocationInterface { /** * {@inheritdoc} */ - public function getAssetsByLocation(AssetInterface $location): array { - if (empty($location->id())) { + public function getAssetsByLocation(array $locations): array { + + // Get location ids. + $location_ids = array_map(function (AssetInterface $location) { + return $location->id(); + }, $locations); + + // Bail if there are no location ids. + if (empty($location_ids)) { return []; } + + // Build query for assets in locations. $query = " -- Select asset IDs from the asset base table. SELECT a.id @@ -261,13 +270,13 @@ class AssetLocation implements AssetLocationInterface { -- Limit results to completed movement logs to the desired location that -- took place before the given timestamp. - WHERE (lfd.is_movement = 1) AND (lfd.status = 'done') AND (lfd.timestamp <= :timestamp) AND (ll.location_target_id = :location_id) + WHERE (lfd.is_movement = 1) AND (lfd.status = 'done') AND (lfd.timestamp <= :timestamp) AND (ll.location_target_id IN (:location_ids[])) -- Exclude records with future log entries. AND lfd2.id IS NULL"; $args = [ ':timestamp' => $this->time->getRequestTime(), - ':location_id' => $location->id(), + ':location_ids[]' => $location_ids, ]; $result = $this->database->query($query, $args)->fetchAll(); $asset_ids = []; diff --git a/modules/core/location/src/AssetLocationInterface.php b/modules/core/location/src/AssetLocationInterface.php index a2c53edad..e89c5d667 100644 --- a/modules/core/location/src/AssetLocationInterface.php +++ b/modules/core/location/src/AssetLocationInterface.php @@ -98,14 +98,14 @@ interface AssetLocationInterface { public function setIntrinsicGeometry(AssetInterface $asset, string $wkt): void; /** - * Get assets that are in a location. + * Get assets that are in locations. * - * @param \Drupal\asset\Entity\AssetInterface $location - * The location asset entity. + * @param \Drupal\asset\Entity\AssetInterface[] $locations + * An array of location assets to lookup. * - * @return array + * @return \Drupal\asset\Entity\AssetInterface[] * Returns an array of assets. */ - public function getAssetsByLocation(AssetInterface $location): array; + public function getAssetsByLocation(array $locations): array; } diff --git a/modules/core/location/src/Plugin/views/argument/AssetLocation.php b/modules/core/location/src/Plugin/views/argument/AssetLocation.php index ccb916271..1021078ed 100644 --- a/modules/core/location/src/Plugin/views/argument/AssetLocation.php +++ b/modules/core/location/src/Plugin/views/argument/AssetLocation.php @@ -81,7 +81,7 @@ class AssetLocation extends ArgumentPluginBase { // See https://www.drupal.org/project/farm/issues/3217168 for more info. /** @var \Drupal\asset\Entity\AssetInterface $location */ $location = $this->entityTypeManager->getStorage('asset')->load($this->argument); - $assets = $this->assetLocation->getAssetsByLocation($location); + $assets = $this->assetLocation->getAssetsByLocation([$location]); $asset_ids = []; foreach ($assets as $asset) { $asset_ids[] = $asset->id(); diff --git a/modules/core/location/tests/src/Kernel/LocationTest.php b/modules/core/location/tests/src/Kernel/LocationTest.php index 566cd175b..4f9df05c3 100644 --- a/modules/core/location/tests/src/Kernel/LocationTest.php +++ b/modules/core/location/tests/src/Kernel/LocationTest.php @@ -348,8 +348,7 @@ class LocationTest extends KernelTestBase { public function testLocationAssets() { // Locations have no assets. - $this->assertEmpty($this->assetLocation->getAssetsByLocation($this->locations[0])); - $this->assertEmpty($this->assetLocation->getAssetsByLocation($this->locations[1])); + $this->assertEmpty($this->assetLocation->getAssetsByLocation([$this->locations[0], $this->locations[1]])); // Create an asset and move it to the first location. /** @var \Drupal\asset\Entity\AssetInterface $first_asset */ @@ -370,8 +369,9 @@ class LocationTest extends KernelTestBase { $first_log->save(); // First location has one asset, second has none. - $this->assertEquals(1, count($this->assetLocation->getAssetsByLocation($this->locations[0]))); - $this->assertEmpty($this->assetLocation->getAssetsByLocation($this->locations[1])); + $this->assertEquals(1, count($this->assetLocation->getAssetsByLocation([$this->locations[0]]))); + $this->assertEmpty($this->assetLocation->getAssetsByLocation([$this->locations[1]])); + $this->assertEquals(1, count($this->assetLocation->getAssetsByLocation([$this->locations[0], $this->locations[1]]))); // Create a second asset and move it to the second location. /** @var \Drupal\asset\Entity\AssetInterface $second_asset */ @@ -392,8 +392,9 @@ class LocationTest extends KernelTestBase { $second_log->save(); // Both locations have one asset. - $this->assertEquals(1, count($this->assetLocation->getAssetsByLocation($this->locations[0]))); - $this->assertEquals(1, count($this->assetLocation->getAssetsByLocation($this->locations[1]))); + $this->assertEquals(1, count($this->assetLocation->getAssetsByLocation([$this->locations[0]]))); + $this->assertEquals(1, count($this->assetLocation->getAssetsByLocation([$this->locations[1]]))); + $this->assertEquals(2, count($this->assetLocation->getAssetsByLocation([$this->locations[0], $this->locations[1]]))); // Create a third log that moves both assets to the first location. /** @var \Drupal\log\Entity\LogInterface $third_log */ @@ -410,8 +411,9 @@ class LocationTest extends KernelTestBase { $third_log->save(); // First location has two assets, second has none. - $this->assertEquals(2, count($this->assetLocation->getAssetsByLocation($this->locations[0]))); - $this->assertEmpty($this->assetLocation->getAssetsByLocation($this->locations[1])); + $this->assertEquals(2, count($this->assetLocation->getAssetsByLocation([$this->locations[0]]))); + $this->assertEmpty($this->assetLocation->getAssetsByLocation([$this->locations[1]])); + $this->assertEquals(2, count($this->assetLocation->getAssetsByLocation([$this->locations[0], $this->locations[1]]))); // Create a fourth log that moves first asset to the second location. /** @var \Drupal\log\Entity\LogInterface $fourth_log */ @@ -427,8 +429,9 @@ class LocationTest extends KernelTestBase { $fourth_log->save(); // Both locations have one asset. - $this->assertEquals(1, count($this->assetLocation->getAssetsByLocation($this->locations[0]))); - $this->assertEquals(1, count($this->assetLocation->getAssetsByLocation($this->locations[1]))); + $this->assertEquals(1, count($this->assetLocation->getAssetsByLocation([$this->locations[0]]))); + $this->assertEquals(1, count($this->assetLocation->getAssetsByLocation([$this->locations[1]]))); + $this->assertEquals(2, count($this->assetLocation->getAssetsByLocation([$this->locations[0], $this->locations[1]]))); // Create a fifth log that moves first asset to the both locations. /** @var \Drupal\log\Entity\LogInterface $fifth_log */ @@ -447,8 +450,9 @@ class LocationTest extends KernelTestBase { $fifth_log->save(); // First location has two asset, second location has one asset. - $this->assertEquals(2, count($this->assetLocation->getAssetsByLocation($this->locations[0]))); - $this->assertEquals(1, count($this->assetLocation->getAssetsByLocation($this->locations[1]))); + $this->assertEquals(2, count($this->assetLocation->getAssetsByLocation([$this->locations[0]]))); + $this->assertEquals(1, count($this->assetLocation->getAssetsByLocation([$this->locations[1]]))); + $this->assertEquals(2, count($this->assetLocation->getAssetsByLocation([$this->locations[0], $this->locations[1]]))); } }