From f2464ffdce2c8f6e834f1001c717405d1d83dd72 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Thu, 7 Nov 2024 16:09:28 -0800 Subject: [PATCH 1/8] notes --- modules/metastore/metastore.module | 16 +++++++++------- .../Factory/MetastoreItemFactoryInterface.php | 5 +++-- modules/metastore/src/LifeCycle/LifeCycle.php | 14 ++++++++++++++ 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/modules/metastore/metastore.module b/modules/metastore/metastore.module index bf3d06ca81..e0ba382891 100644 --- a/modules/metastore/metastore.module +++ b/modules/metastore/metastore.module @@ -13,8 +13,13 @@ use Drupal\metastore\Storage\MetastoreEntityStorageInterface; /** * Implements hook_entity_load(). + * + * @see Drupal\metastore\LifeCycle\LifeCycle::datasetLoad() + * @see Drupal\metastore\LifeCycle\LifeCycle::distributionLoad() */ function metastore_entity_load(array $entities) { + // @todo Being able to process multiple entities at once is a tremendous + // benefit since it allows you to reduce the number of DB queries involved. foreach($entities as $entity) { metastore_data_lifecycle($entity, "load"); } @@ -63,19 +68,14 @@ function resource_mapper_new_revision() { * Create a LifeCycle object. * * @param Drupal\Core\Entity\ContentEntityInterface $entity - * - * @return Drupal\metastore\LifeCycle\Data - * LifeCycle object. */ -function metastore_data_lifecycle(EntityInterface $entity, string $stage) { - +function metastore_data_lifecycle(EntityInterface $entity, string $stage): void { if (metastore_entity_is_valid_item($entity)) { + /** @var \Drupal\metastore\Factory\MetastoreEntityItemFactoryInterface $itemFactory */ $itemFactory = \Drupal::service('dkan.metastore.metastore_item_factory'); $metastoreItem = $itemFactory->wrap($entity); \Drupal::service('dkan.metastore.lifecycle')->go($stage, $metastoreItem); } - - return FALSE; } /** @@ -91,6 +91,7 @@ function metastore_entity_is_valid_item(EntityInterface $entity) { $storageClass = \Drupal::service('dkan.metastore.storage')::getStorageClass(); // If the storage class used implements the entity storage interface, continue. + // @todo Should we look at the entity's storage class instead? if (!is_a($storageClass, MetastoreEntityStorageInterface::class, true)) { return FALSE; } @@ -105,6 +106,7 @@ function metastore_entity_is_valid_item(EntityInterface $entity) { if (in_array($entity->bundle(), $storageBundles)) { return TRUE; } + return FALSE; } /** diff --git a/modules/metastore/src/Factory/MetastoreItemFactoryInterface.php b/modules/metastore/src/Factory/MetastoreItemFactoryInterface.php index e33cb283d4..3284393945 100644 --- a/modules/metastore/src/Factory/MetastoreItemFactoryInterface.php +++ b/modules/metastore/src/Factory/MetastoreItemFactoryInterface.php @@ -5,6 +5,7 @@ use Contracts\FactoryInterface; use Drupal\Core\Entity\EntityRepository; use Drupal\Core\Entity\EntityTypeManager; +use Drupal\metastore\MetastoreItemInterface; /** * Interface MetastoreItemFactoryInterface. @@ -44,10 +45,10 @@ public function getInstance(string $identifier, array $config = []); * @param mixed $input * Any object that can be wrapped as a metastore item. For instance, a node. * - * @return Drupal\metastore\MetastoreItemInterface + * @return \Drupal\metastore\MetastoreItemInterface * A metastore item interface compliant object. */ - public function wrap(mixed $input); + public function wrap(mixed $input): MetastoreItemInterface; /** * Return list cache tags for metastore items. diff --git a/modules/metastore/src/LifeCycle/LifeCycle.php b/modules/metastore/src/LifeCycle/LifeCycle.php index 0b74cd8cb2..83430b629a 100644 --- a/modules/metastore/src/LifeCycle/LifeCycle.php +++ b/modules/metastore/src/LifeCycle/LifeCycle.php @@ -122,6 +122,8 @@ public function __construct( * Stage or hook name for execution. * @param \Drupal\metastore\MetastoreItemInterface $data * Metastore item object. + * + * @todo Just call the methods from the hooks instead of this. */ public function go(string $stage, MetastoreItemInterface $data): void { // Removed dashes from schema ID since function names can't include dashes. @@ -149,6 +151,12 @@ protected function datasetPredelete(MetastoreItemInterface $data) { /** * Dataset load. + * + * @todo The sheer overhead here has struck me. This happens on every dataset + * node entity in $node_storage->loadMultiple() whether it needs to or not, + * via metastore_entity_load(). + * + * @see \metastore_entity_load() */ protected function datasetLoad(MetastoreItemInterface $data) { $metadata = $data->getMetaData(); @@ -179,6 +187,12 @@ protected function datasetUpdate(MetastoreItemInterface $data) { * @todo For consistency, this should either be abstracted so that it is not * so tightly coupled with the distribution schema, or we should better * document that DKAN only supports DCAT standard. + * + * @todo Make this method actually responsive to hook_entity_load(), so that + * we can benefit from loading multiple entities simultaneously in fewer + * queries. + * + * @see metastore_entity_load() */ protected function distributionLoad(MetastoreItemInterface $data) { $metadata = $data->getMetaData(); From c2898765bd6ea589159dde4549f804f9f3a3e3d0 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Fri, 8 Nov 2024 10:22:06 -0800 Subject: [PATCH 2/8] some changes --- .../src/Storage/AbstractDatabaseTable.php | 2 + .../datastore/src/Service/ImportService.php | 9 +++-- .../src/Controller/MetastoreController.php | 40 ++++++++++--------- .../Factory/MetastoreItemFactoryInterface.php | 6 +-- modules/metastore/src/NodeWrapper/Data.php | 2 + .../src/NodeWrapper/NodeDataFactory.php | 18 +++++---- modules/metastore/src/Storage/Data.php | 7 +++- .../tests/src/Unit/NodeWrapper/DataTest.php | 8 +++- 8 files changed, 56 insertions(+), 36 deletions(-) diff --git a/modules/common/src/Storage/AbstractDatabaseTable.php b/modules/common/src/Storage/AbstractDatabaseTable.php index e477b179e0..10cf083757 100644 --- a/modules/common/src/Storage/AbstractDatabaseTable.php +++ b/modules/common/src/Storage/AbstractDatabaseTable.php @@ -305,6 +305,8 @@ protected function tableExist($table_name): bool { */ protected function tableCreate($table_name, $schema) { // Opportunity to further alter the schema before table creation. + // @todo Give this event its own event class. Pass the schema array by + // reference. $schema = $this->dispatchEvent(self::EVENT_TABLE_CREATE, $schema); $this->connection->schema()->createTable($table_name, $schema); diff --git a/modules/datastore/src/Service/ImportService.php b/modules/datastore/src/Service/ImportService.php index d826fa01f4..f2208fd6ec 100644 --- a/modules/datastore/src/Service/ImportService.php +++ b/modules/datastore/src/Service/ImportService.php @@ -17,9 +17,10 @@ /** * Datastore importer. * - * @todo This class has state and is not actually a service because it holds - * state. Have import() take an argument of a resource, instead of storing it - * as a property. + * Use the factory service (dkan.datastore.service.factory.import) to generate + * these. + * + * @see \Drupal\datastore\Service\Factory\ImportServiceFactory::getInstance */ class ImportService { @@ -144,7 +145,7 @@ protected function getResource(): DataResource { } /** - * Import. + * Import resources into storage. */ public function import() { $result = $this->getImporter()->run(); diff --git a/modules/metastore/src/Controller/MetastoreController.php b/modules/metastore/src/Controller/MetastoreController.php index 4887270608..03b9846776 100644 --- a/modules/metastore/src/Controller/MetastoreController.php +++ b/modules/metastore/src/Controller/MetastoreController.php @@ -36,7 +36,7 @@ class MetastoreController implements ContainerInjectionInterface { * * @var \Drupal\metastore\MetastoreService */ - private MetastoreService $service; + private MetastoreService $metastoreService; /** * Metastore dataset docs service. @@ -68,9 +68,13 @@ public static function create(ContainerInterface $container) { /** * Constructor. */ - public function __construct(MetastoreApiResponse $apiResponse, MetastoreService $service, DatasetApiDocs $docs) { + public function __construct( + MetastoreApiResponse $apiResponse, + MetastoreService $metastoreService, + DatasetApiDocs $docs, + ) { $this->apiResponse = $apiResponse; - $this->service = $service; + $this->metastoreService = $metastoreService; $this->docs = $docs; } @@ -78,7 +82,7 @@ public function __construct(MetastoreApiResponse $apiResponse, MetastoreService * Get schemas. */ public function getSchemas() { - return $this->apiResponse->cachedJsonResponse($this->service->getSchemas()); + return $this->apiResponse->cachedJsonResponse($this->metastoreService->getSchemas()); } /** @@ -86,7 +90,7 @@ public function getSchemas() { */ public function getSchema(string $identifier) { try { - return $this->apiResponse->cachedJsonResponse($this->service->getSchema($identifier)); + return $this->apiResponse->cachedJsonResponse($this->metastoreService->getSchema($identifier)); } catch (\Exception $e) { return $this->getResponseFromException($e, 404); @@ -109,10 +113,10 @@ public function getAll(string $schema_id, Request $request) { $output = array_map(function ($object) use ($keepRefs) { $modified_object = $keepRefs - ? $this->service->swapReferences($object) - : $this->service->removeReferences($object); + ? $this->metastoreService->swapReferences($object) + : $this->metastoreService->removeReferences($object); return (object) $modified_object->get('$'); - }, $this->service->getAll($schema_id)); + }, $this->metastoreService->getAll($schema_id)); $output = array_values($output); return $this->apiResponse->cachedJsonResponse($output, 200, [$schema_id], $request->query); @@ -136,9 +140,9 @@ public function getAll(string $schema_id, Request $request) { */ public function get(string $schema_id, string $identifier, Request $request) { try { - $object = $this->service->get($schema_id, $identifier); + $object = $this->metastoreService->get($schema_id, $identifier); if ($this->wantObjectWithReferences($request)) { - $object = $this->service->swapReferences($object); + $object = $this->metastoreService->swapReferences($object); } else { $object = MetastoreService::removeReferences($object); @@ -184,8 +188,8 @@ public function post(string $schema_id, Request $request) { try { $data = $request->getContent(); $this->checkIdentifier($data); - $data = $this->service->getValidMetadataFactory()->get($data, $schema_id, ['method' => 'POST']); - $identifier = $this->service->post($schema_id, $data); + $data = $this->metastoreService->getValidMetadataFactory()->get($data, $schema_id, ['method' => 'POST']); + $identifier = $this->metastoreService->post($schema_id, $data); return $this->apiResponse->cachedJsonResponse([ "endpoint" => "{$request->getRequestUri()}/{$identifier}", "identifier" => $identifier, @@ -214,7 +218,7 @@ public function post(string $schema_id, Request $request) { */ public function publish(string $schema_id, string $identifier, Request $request) { try { - $this->service->publish($schema_id, $identifier); + $this->metastoreService->publish($schema_id, $identifier); return $this->apiResponse->cachedJsonResponse((object) [ "endpoint" => "{$request->getRequestUri()}/publish", "identifier" => $identifier, @@ -245,8 +249,8 @@ public function put($schema_id, string $identifier, Request $request) { try { $data = $request->getContent(); $this->checkIdentifier($data, $identifier); - $data = $this->service->getValidMetadataFactory()->get($data, $schema_id); - $info = $this->service->put($schema_id, $identifier, $data); + $data = $this->metastoreService->getValidMetadataFactory()->get($data, $schema_id); + $info = $this->metastoreService->put($schema_id, $identifier, $data); $code = ($info['new'] == TRUE) ? 201 : 200; return $this->apiResponse->cachedJsonResponse( [ @@ -291,7 +295,7 @@ public function patch($schema_id, $identifier, Request $request) { } $this->checkIdentifier($data, $identifier); - $this->service->patch($schema_id, $identifier, $data); + $this->metastoreService->patch($schema_id, $identifier, $data); return $this->apiResponse->cachedJsonResponse((object) [ "endpoint" => $request->getRequestUri(), "identifier" => $identifier, @@ -318,7 +322,7 @@ public function patch($schema_id, $identifier, Request $request) { */ public function delete($schema_id, $identifier) { try { - $this->service->delete($schema_id, $identifier); + $this->metastoreService->delete($schema_id, $identifier); return $this->apiResponse->cachedJsonResponse((object) ["message" => "Dataset {$identifier} has been deleted."]); } catch (\Exception $e) { @@ -334,7 +338,7 @@ public function delete($schema_id, $identifier) { */ public function getCatalog() : JsonResponse { try { - return $this->apiResponse->cachedJsonResponse($this->service->getCatalog()); + return $this->apiResponse->cachedJsonResponse($this->metastoreService->getCatalog()); } catch (\Exception $e) { return $this->getResponseFromException($e); diff --git a/modules/metastore/src/Factory/MetastoreItemFactoryInterface.php b/modules/metastore/src/Factory/MetastoreItemFactoryInterface.php index 3284393945..ca121dae2f 100644 --- a/modules/metastore/src/Factory/MetastoreItemFactoryInterface.php +++ b/modules/metastore/src/Factory/MetastoreItemFactoryInterface.php @@ -10,9 +10,9 @@ /** * Interface MetastoreItemFactoryInterface. * - * Used for service dkan.metastore.metastore_item_factory. Override the service + * Used for service dkan.metastore.metastore_item_factory. Decorate the service * to use different logic for producing a MetastoreItemInterface object from - * just an indentifier. + * just an identifier. */ interface MetastoreItemFactoryInterface extends FactoryInterface { @@ -37,7 +37,7 @@ public function __construct(EntityRepository $entityRepository, EntityTypeManage * @return \Drupal\metastore\MetastoreItemInterface * A metastore item object. */ - public function getInstance(string $identifier, array $config = []); + public function getInstance(string $identifier, array $config = []): MetastoreItemInterface; /** * Wrap an arbitrary object as a metastore item interface compliant object. diff --git a/modules/metastore/src/NodeWrapper/Data.php b/modules/metastore/src/NodeWrapper/Data.php index 2d3cd7980c..80dbe59bb5 100644 --- a/modules/metastore/src/NodeWrapper/Data.php +++ b/modules/metastore/src/NodeWrapper/Data.php @@ -81,6 +81,8 @@ public function getCacheMaxAge() { /** * Private. + * + * @todo Needing to call fix() on every method seems like a code smell. */ private function fix() { $this->fixDataType(); diff --git a/modules/metastore/src/NodeWrapper/NodeDataFactory.php b/modules/metastore/src/NodeWrapper/NodeDataFactory.php index 470ebfb288..32e23a362c 100644 --- a/modules/metastore/src/NodeWrapper/NodeDataFactory.php +++ b/modules/metastore/src/NodeWrapper/NodeDataFactory.php @@ -5,6 +5,7 @@ use Drupal\Core\Entity\EntityRepository; use Drupal\Core\Entity\EntityTypeManagerInterface; use Drupal\metastore\Factory\MetastoreEntityItemFactoryInterface; +use Drupal\metastore\MetastoreItemInterface; /** * Class NodeDataFactory. @@ -48,25 +49,26 @@ public function __construct(EntityRepository $entityRepository, EntityTypeManage * @param array $config * Optional config from interface, not used. * - * @return Data + * @return \Drupal\metastore\MetastoreItemInterface * Metastore data node object. */ - public function getInstance(string $identifier, array $config = []) { - $dataNode = $this->entityRepository->loadEntityByUuid("node", $identifier); - return new Data($dataNode, $this->entityTypeManager); + public function getInstance(string $identifier, array $config = []): MetastoreItemInterface { + return $this->wrap( + $this->entityRepository->loadEntityByUuid('node', $identifier) + ); } /** * Create a metastore node data object from a node object. * - * @param mixed $dataNode + * @param mixed $input * A data node. * - * @return Data + * @return \Drupal\metastore\MetastoreItemInterface * Metastore data node object. */ - public function wrap($dataNode) { - return new Data($dataNode, $this->entityTypeManager); + public function wrap($input): MetastoreItemInterface { + return new Data($input, $this->entityTypeManager); } /** diff --git a/modules/metastore/src/Storage/Data.php b/modules/metastore/src/Storage/Data.php index dd9742dc0b..d9b32320d9 100644 --- a/modules/metastore/src/Storage/Data.php +++ b/modules/metastore/src/Storage/Data.php @@ -357,11 +357,14 @@ private function updateExistingEntity(ContentEntityInterface $entity, $data): ?s $entity->{$this->metadataField} = $new_data; // Dkan publishing's default moderation state. + // @todo Honor existing entity's moderation state: + // https://github.com/GetDKAN/dkan/issues/4337 $entity->set('moderation_state', $this->getDefaultModerationState()); if ($entity instanceof RevisionLogInterface) { - $entity->setRevisionLogMessage("Updated on " . (new \DateTimeImmutable())->format(\DateTimeImmutable::ATOM)); - $entity->setRevisionCreationTime(time()); + $time = new \DateTimeImmutable(); + $entity->setRevisionLogMessage("Updated on " . $time->format(\DateTimeImmutable::ATOM)); + $entity->setRevisionCreationTime($time->getTimestamp()); } $entity->save(); diff --git a/modules/metastore/tests/src/Unit/NodeWrapper/DataTest.php b/modules/metastore/tests/src/Unit/NodeWrapper/DataTest.php index 34f2e7aed9..1bebabde50 100644 --- a/modules/metastore/tests/src/Unit/NodeWrapper/DataTest.php +++ b/modules/metastore/tests/src/Unit/NodeWrapper/DataTest.php @@ -17,9 +17,15 @@ use Symfony\Component\DependencyInjection\Container; /** - * Testing the NodeWrapper. + * @coversDefaultClass \Drupal\metastore\NodeWrapper\Data + * @covers \Drupal\metastore\NodeWrapper\Data + * + * @group dkan + * @group metastore + * @group unit */ class DataTest extends TestCase { + public function testGetLatestRevisionGetUsAWrapper() { $node = (new Chain($this)) ->add(Node::class, 'bundle', 'data') From 83e6f780c0874ad587f47aeeb16bd2da78cb6ea7 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Sun, 17 Nov 2024 14:17:06 -0800 Subject: [PATCH 3/8] hook_entity_type_op --- modules/metastore/metastore.module | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/modules/metastore/metastore.module b/modules/metastore/metastore.module index e0ba382891..672110b0e6 100644 --- a/modules/metastore/metastore.module +++ b/modules/metastore/metastore.module @@ -12,38 +12,38 @@ use Drupal\metastore\MetastoreService; use Drupal\metastore\Storage\MetastoreEntityStorageInterface; /** - * Implements hook_entity_load(). + * Implements hook_ENTITY_TYPE_load(). * * @see Drupal\metastore\LifeCycle\LifeCycle::datasetLoad() * @see Drupal\metastore\LifeCycle\LifeCycle::distributionLoad() */ -function metastore_entity_load(array $entities) { +function metastore_node_load(array $entities) { // @todo Being able to process multiple entities at once is a tremendous // benefit since it allows you to reduce the number of DB queries involved. foreach($entities as $entity) { - metastore_data_lifecycle($entity, "load"); + metastore_data_lifecycle($entity, 'load'); } } /** - * Implements hook_entity_presave(). + * Implements hook_ENTITY_TYPE_presave(). */ -function metastore_entity_presave(EntityInterface $entity) { - metastore_data_lifecycle($entity, "presave"); +function metastore_node_presave(EntityInterface $entity) { + metastore_data_lifecycle($entity, 'presave'); } /** - * Implements hook_entity_predelete(). + * Implements hook_ENTITY_TYPE_predelete(). */ -function metastore_entity_predelete(EntityInterface $entity) { - metastore_data_lifecycle($entity, "predelete"); +function metastore_node_predelete(EntityInterface $entity) { + metastore_data_lifecycle($entity, 'predelete'); } /** - * Implements hook_entity_update(). + * Implements hook_ENTITY_TYPE_update(). */ -function metastore_entity_update(EntityInterface $entity) { - metastore_data_lifecycle($entity, "update"); +function metastore_node_update(EntityInterface $entity) { + metastore_data_lifecycle($entity, 'update'); } /** From c4f033e524b70f95c539e84c8830b0599ce9b3c5 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Wed, 20 Nov 2024 09:22:00 -0800 Subject: [PATCH 4/8] some changes --- modules/metastore/src/LifeCycle/LifeCycle.php | 16 +++++----- modules/metastore/src/NodeWrapper/Data.php | 32 ++++++++++--------- 2 files changed, 25 insertions(+), 23 deletions(-) diff --git a/modules/metastore/src/LifeCycle/LifeCycle.php b/modules/metastore/src/LifeCycle/LifeCycle.php index 825ac71e05..96d7f6e403 100644 --- a/modules/metastore/src/LifeCycle/LifeCycle.php +++ b/modules/metastore/src/LifeCycle/LifeCycle.php @@ -152,11 +152,11 @@ protected function datasetPredelete(MetastoreItemInterface $data) { /** * Dataset load. * - * @todo The sheer overhead here has struck me. This happens on every dataset - * node entity in $node_storage->loadMultiple() whether it needs to or not, - * via metastore_entity_load(). + * @todo This behavior should be on-demand instead of always happening when + * the node loads, since not all dataset nodes will need to be + * dereferenced. * - * @see \metastore_entity_load() + * @see \metastore_node_load() */ protected function datasetLoad(MetastoreItemInterface $data) { $metadata = $data->getMetaData(); @@ -188,11 +188,11 @@ protected function datasetUpdate(MetastoreItemInterface $data) { * so tightly coupled with the distribution schema, or we should better * document that DKAN only supports DCAT standard. * - * @todo Make this method actually responsive to hook_entity_load(), so that - * we can benefit from loading multiple entities simultaneously in fewer - * queries. + * @todo This behavior should be on-demand instead of always happening when + * the node loads, since not all node loads will need dereferenced download + * URLs. * - * @see metastore_entity_load() + * @see \metastore_node_load() */ protected function distributionLoad(MetastoreItemInterface $data) { $metadata = $data->getMetaData(); diff --git a/modules/metastore/src/NodeWrapper/Data.php b/modules/metastore/src/NodeWrapper/Data.php index 80dbe59bb5..f550c80bbc 100644 --- a/modules/metastore/src/NodeWrapper/Data.php +++ b/modules/metastore/src/NodeWrapper/Data.php @@ -3,53 +3,53 @@ namespace Drupal\metastore\NodeWrapper; use Drupal\Core\Entity\EntityInterface; -use Drupal\common\Exception\DataNodeLifeCycleEntityValidationException; +use Drupal\Core\Entity\EntityStorageInterface; use Drupal\Core\Entity\EntityTypeManagerInterface; +use Drupal\common\Exception\DataNodeLifeCycleEntityValidationException; use Drupal\metastore\MetastoreItemInterface; -use Drupal\node\Entity\Node; +use Drupal\node\NodeInterface; /** * MetastoreItem object that wraps a data node, provides additional methods. + * + * Generate these objects using the factory: + * dkan.metastore.metastore_item_factory. + * + * @see \Drupal\metastore\NodeWrapper\NodeDataFactory::getInstance() */ class Data implements MetastoreItemInterface { /** * Node. * - * @var \Drupal\node\Entity\Node + * @var \Drupal\Core\Entity\EntityInterface */ - protected $node; - - /** - * Referenced raw metadata string. - * - * @var string - */ - protected $rawMetadata; + protected EntityInterface $node; /** * Entity Type Manager. * * @var \Drupal\Core\Entity\EntityTypeManagerInterface */ - private $entityTypeManager; + private EntityTypeManagerInterface $entityTypeManager; /** * Entity Node Storage. * * @var \Drupal\Core\Entity\EntityStorageInterface */ - private $nodeStorage; + private EntityStorageInterface $nodeStorage; /** * Constructor. * * @param \Drupal\Core\Entity\EntityInterface $entity - * A Drupal entity. + * A Drupal entity. Must be a Data node. * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager * Entity Type Manager service. * * @throws \Drupal\common\Exception\DataNodeLifeCycleEntityValidationException + * Thrown when the entity is not a Data node. */ public function __construct(EntityInterface $entity, EntityTypeManagerInterface $entityTypeManager) { $this->validate($entity); @@ -169,7 +169,7 @@ public function isNew() { * Private. */ private function validate(EntityInterface $entity) { - if (!($entity instanceof Node)) { + if (!($entity instanceof NodeInterface)) { throw new DataNodeLifeCycleEntityValidationException("We only work with nodes."); } @@ -197,6 +197,8 @@ public function getSchemaId() { /** * Private. + * + * @todo Why do we do this? */ private function saveRawMetadata() { // Temporarily save the raw json metadata, for later use. From 8b4f05a5338ca72296115a18606371d9a270b4bf Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Wed, 20 Nov 2024 10:56:18 -0800 Subject: [PATCH 5/8] docblock --- modules/metastore/metastore.module | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/modules/metastore/metastore.module b/modules/metastore/metastore.module index 672110b0e6..9a6ea194e1 100644 --- a/modules/metastore/metastore.module +++ b/modules/metastore/metastore.module @@ -65,9 +65,12 @@ function resource_mapper_new_revision() { } /** - * Create a LifeCycle object. + * Send the hook off to the lifecycle service. * * @param Drupal\Core\Entity\ContentEntityInterface $entity + * The entity in question. + * @param string $stage + * The entity hook stage, such as 'load' or 'update'. */ function metastore_data_lifecycle(EntityInterface $entity, string $stage): void { if (metastore_entity_is_valid_item($entity)) { From fd523825c58496d21f18f4f02949dea29521f518 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Thu, 26 Dec 2024 10:25:55 -0700 Subject: [PATCH 6/8] moves more responsibilities to LifeCycle --- modules/metastore/metastore.module | 56 +++++++------------ .../Factory/MetastoreItemFactoryInterface.php | 6 +- modules/metastore/src/LifeCycle/LifeCycle.php | 52 +++++++++++++++-- .../metastore/src/MetastoreItemInterface.php | 5 +- 4 files changed, 74 insertions(+), 45 deletions(-) diff --git a/modules/metastore/metastore.module b/modules/metastore/metastore.module index 9a6ea194e1..8d8541fcf4 100644 --- a/modules/metastore/metastore.module +++ b/modules/metastore/metastore.module @@ -18,32 +18,28 @@ use Drupal\metastore\Storage\MetastoreEntityStorageInterface; * @see Drupal\metastore\LifeCycle\LifeCycle::distributionLoad() */ function metastore_node_load(array $entities) { - // @todo Being able to process multiple entities at once is a tremendous - // benefit since it allows you to reduce the number of DB queries involved. - foreach($entities as $entity) { - metastore_data_lifecycle($entity, 'load'); - } + metastore_data_lifecycle($entities, 'load'); } /** * Implements hook_ENTITY_TYPE_presave(). */ function metastore_node_presave(EntityInterface $entity) { - metastore_data_lifecycle($entity, 'presave'); + metastore_data_lifecycle([$entity], 'presave'); } /** * Implements hook_ENTITY_TYPE_predelete(). */ function metastore_node_predelete(EntityInterface $entity) { - metastore_data_lifecycle($entity, 'predelete'); + metastore_data_lifecycle([$entity], 'predelete'); } /** * Implements hook_ENTITY_TYPE_update(). */ function metastore_node_update(EntityInterface $entity) { - metastore_data_lifecycle($entity, 'update'); + metastore_data_lifecycle([$entity], 'update'); } /** @@ -67,17 +63,21 @@ function resource_mapper_new_revision() { /** * Send the hook off to the lifecycle service. * - * @param Drupal\Core\Entity\ContentEntityInterface $entity - * The entity in question. + * @param Drupal\Core\Entity\ContentEntityInterface[] $entities + * The entities in question. * @param string $stage * The entity hook stage, such as 'load' or 'update'. */ -function metastore_data_lifecycle(EntityInterface $entity, string $stage): void { - if (metastore_entity_is_valid_item($entity)) { - /** @var \Drupal\metastore\Factory\MetastoreEntityItemFactoryInterface $itemFactory */ - $itemFactory = \Drupal::service('dkan.metastore.metastore_item_factory'); - $metastoreItem = $itemFactory->wrap($entity); - \Drupal::service('dkan.metastore.lifecycle')->go($stage, $metastoreItem); +function metastore_data_lifecycle(array $entities, string $stage): void { + /** @var \Drupal\metastore\LifeCycle\LifeCycle $life_cycle */ + $life_cycle = \Drupal::service('dkan.metastore.lifecycle'); + /** @var \Drupal\metastore\Factory\MetastoreEntityItemFactoryInterface $itemFactory */ + $itemFactory = \Drupal::service('dkan.metastore.metastore_item_factory'); + foreach ($entities as $entity) { + if ($life_cycle->entityIsValidItem($entity)) { + $metastoreItem = $itemFactory->wrap($entity); + $life_cycle->go($stage, $metastoreItem); + } } } @@ -89,27 +89,13 @@ function metastore_data_lifecycle(EntityInterface $entity, string $stage): void * * @return bool * Returns true if the entity is used by the metastore. + * + * @deprecated Use LifeCycle::entityIsValidItem(). */ function metastore_entity_is_valid_item(EntityInterface $entity) { - $storageClass = \Drupal::service('dkan.metastore.storage')::getStorageClass(); - - // If the storage class used implements the entity storage interface, continue. - // @todo Should we look at the entity's storage class instead? - if (!is_a($storageClass, MetastoreEntityStorageInterface::class, true)) { - return FALSE; - } - - $storageEntityType = \Drupal::service('dkan.metastore.metastore_item_factory')::getEntityType(); - $storageBundles = \Drupal::service('dkan.metastore.metastore_item_factory')::getBundles(); - - // If the type and bundle are correct, return true. - if ($entity->getEntityTypeId() != $storageEntityType) { - return FALSE; - } - if (in_array($entity->bundle(), $storageBundles)) { - return TRUE; - } - return FALSE; + /** @var \Drupal\metastore\LifeCycle\LifeCycle $life_cycle */ + $life_cycle = \Drupal::service('dkan.metastore.lifecycle'); + return $life_cycle->entityIsValidItem($entity); } /** diff --git a/modules/metastore/src/Factory/MetastoreItemFactoryInterface.php b/modules/metastore/src/Factory/MetastoreItemFactoryInterface.php index ca121dae2f..ef7cf1e6ee 100644 --- a/modules/metastore/src/Factory/MetastoreItemFactoryInterface.php +++ b/modules/metastore/src/Factory/MetastoreItemFactoryInterface.php @@ -42,13 +42,13 @@ public function getInstance(string $identifier, array $config = []): MetastoreIt /** * Wrap an arbitrary object as a metastore item interface compliant object. * - * @param mixed $input + * @param object $input * Any object that can be wrapped as a metastore item. For instance, a node. * * @return \Drupal\metastore\MetastoreItemInterface - * A metastore item interface compliant object. + * A wrapper that implements MetastoreItemInterface. */ - public function wrap(mixed $input): MetastoreItemInterface; + public function wrap(object $input): MetastoreItemInterface; /** * Return list cache tags for metastore items. diff --git a/modules/metastore/src/LifeCycle/LifeCycle.php b/modules/metastore/src/LifeCycle/LifeCycle.php index 96d7f6e403..450bafbc6b 100644 --- a/modules/metastore/src/LifeCycle/LifeCycle.php +++ b/modules/metastore/src/LifeCycle/LifeCycle.php @@ -10,6 +10,8 @@ use Drupal\Component\Plugin\Exception\PluginNotFoundException; use Drupal\Core\Config\ConfigFactory; use Drupal\Core\Datetime\DateFormatter; +use Drupal\Core\Entity\ContentEntityInterface; +use Drupal\Core\Entity\EntityInterface; use Drupal\Core\Queue\QueueFactory; use Drupal\Core\StreamWrapper\StreamWrapperManager; use Drupal\metastore\MetastoreItemInterface; @@ -19,6 +21,7 @@ use Drupal\metastore\Reference\Referencer; use Drupal\metastore\ResourceMapper; use Drupal\metastore\Storage\DataFactory; +use Drupal\metastore\Storage\MetastoreEntityStorageInterface; /** * Abstraction of logic used in entity hooks. @@ -103,7 +106,7 @@ public function __construct( DateFormatter $dateFormatter, DataFactory $dataFactory, QueueFactory $queueFactory, - ConfigFactory $configFactory + ConfigFactory $configFactory, ) { $this->referencer = $referencer; $this->dereferencer = $dereferencer; @@ -115,26 +118,63 @@ public function __construct( $this->configFactory = $configFactory; } + /** + * Check if the entity is part of the metastore. + * + * @param \Drupal\Core\Entity\ContentEntityInterface $entity + * A Drupal content entity. + * + * @return bool + * Returns true if the entity is used by the metastore. + */ + public function entityIsValidItem(ContentEntityInterface $entity) { + $storageClass = \Drupal::service('dkan.metastore.storage')::getStorageClass(); + + // If the storage class used implements the entity storage interface, continue. + // @todo Should we look at the entity's storage class instead? + if (!is_a($storageClass, MetastoreEntityStorageInterface::class, TRUE)) { + return FALSE; + } + + // @todo Inject these. + $storageEntityType = \Drupal::service('dkan.metastore.metastore_item_factory')::getEntityType(); + $storageBundles = \Drupal::service('dkan.metastore.metastore_item_factory')::getBundles(); + + // If the type and bundle are correct, return true. + if ($entity->getEntityTypeId() != $storageEntityType) { + return FALSE; + } + if (in_array($entity->bundle(), $storageBundles)) { + return TRUE; + } + return FALSE; + } + /** * Entry point for LifeCycle functions. * + * Based on the schema of the $data object and the $stage, we generate a + * method name. If that method name exists on this class, we call it. + * Example: Stage 'load' for a dataset metastore item becomes 'datasetLoad'. + * + * Currently, this method handles hook implementations for Data nodes via + * wrappers, but might be expected to handle arbitrary entities in the + * future. + * * @param string $stage * Stage or hook name for execution. * @param \Drupal\metastore\MetastoreItemInterface $data * Metastore item object. - * - * @todo Just call the methods from the hooks instead of this. */ public function go(string $stage, MetastoreItemInterface $data): void { // Removed dashes from schema ID since function names can't include dashes. $schema_id = str_replace('-', '', $data->getSchemaId()); - $stage = ucwords($stage); // Build method name from schema ID and stage. - $method = "{$schema_id}{$stage}"; + $method = $schema_id . ucwords($stage); // Ensure a method exists for this life cycle stage. if (method_exists($this, $method)) { // Call life cycle method on metastore item. - $this->{$method}($data); + $this->$method($data); } } diff --git a/modules/metastore/src/MetastoreItemInterface.php b/modules/metastore/src/MetastoreItemInterface.php index 22d5613bbd..4e9376033f 100644 --- a/modules/metastore/src/MetastoreItemInterface.php +++ b/modules/metastore/src/MetastoreItemInterface.php @@ -25,7 +25,10 @@ public function getIdentifier(); public function getRawMetadata(); /** - * Protected. + * Get the node schema identifier. + * + * @return string + * The Data node schema identifier, such as 'dataset' or 'distribution'. */ public function getSchemaId(); From c04e0f67e71bba63d774034c9d1ba081f9e83b7a Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Thu, 26 Dec 2024 10:37:21 -0700 Subject: [PATCH 7/8] cs --- modules/metastore/src/LifeCycle/LifeCycle.php | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/modules/metastore/src/LifeCycle/LifeCycle.php b/modules/metastore/src/LifeCycle/LifeCycle.php index 450bafbc6b..ebd519b9d7 100644 --- a/modules/metastore/src/LifeCycle/LifeCycle.php +++ b/modules/metastore/src/LifeCycle/LifeCycle.php @@ -2,18 +2,17 @@ namespace Drupal\metastore\LifeCycle; -use Drupal\common\EventDispatcherTrait; -use Drupal\common\DataResource; -use Drupal\common\Exception\DataNodeLifeCycleEntityValidationException; -use Drupal\common\UrlHostTokenResolver; use Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException; use Drupal\Component\Plugin\Exception\PluginNotFoundException; use Drupal\Core\Config\ConfigFactory; use Drupal\Core\Datetime\DateFormatter; use Drupal\Core\Entity\ContentEntityInterface; -use Drupal\Core\Entity\EntityInterface; use Drupal\Core\Queue\QueueFactory; use Drupal\Core\StreamWrapper\StreamWrapperManager; +use Drupal\common\DataResource; +use Drupal\common\EventDispatcherTrait; +use Drupal\common\Exception\DataNodeLifeCycleEntityValidationException; +use Drupal\common\UrlHostTokenResolver; use Drupal\metastore\MetastoreItemInterface; use Drupal\metastore\Reference\Dereferencer; use Drupal\metastore\Reference\MetastoreUrlGenerator; @@ -128,9 +127,11 @@ public function __construct( * Returns true if the entity is used by the metastore. */ public function entityIsValidItem(ContentEntityInterface $entity) { + // @todo Inject this. $storageClass = \Drupal::service('dkan.metastore.storage')::getStorageClass(); - // If the storage class used implements the entity storage interface, continue. + // If the storage class used implements the entity storage interface, + // continue. // @todo Should we look at the entity's storage class instead? if (!is_a($storageClass, MetastoreEntityStorageInterface::class, TRUE)) { return FALSE; From 539973116a8ab135792aa816bb98ec66f747f9be Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Thu, 2 Jan 2025 11:53:17 -0800 Subject: [PATCH 8/8] reference doc --- .../src/Controller/MetastoreController.php | 40 +++++++++---------- .../src/Reference/ReferenceLookup.php | 3 ++ 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/modules/metastore/src/Controller/MetastoreController.php b/modules/metastore/src/Controller/MetastoreController.php index b679ccdef4..075ee389f2 100644 --- a/modules/metastore/src/Controller/MetastoreController.php +++ b/modules/metastore/src/Controller/MetastoreController.php @@ -32,7 +32,7 @@ class MetastoreController implements ContainerInjectionInterface { /** * Metastore service. */ - private MetastoreService $metastoreService; + private MetastoreService $service; /** * Metastore dataset docs service. @@ -60,13 +60,9 @@ public static function create(ContainerInterface $container) { /** * Constructor. */ - public function __construct( - MetastoreApiResponse $apiResponse, - MetastoreService $metastoreService, - DatasetApiDocs $docs, - ) { + public function __construct(MetastoreApiResponse $apiResponse, MetastoreService $service, DatasetApiDocs $docs) { $this->apiResponse = $apiResponse; - $this->metastoreService = $metastoreService; + $this->service = $service; $this->docs = $docs; } @@ -74,7 +70,7 @@ public function __construct( * Get schemas. */ public function getSchemas() { - return $this->apiResponse->cachedJsonResponse($this->metastoreService->getSchemas()); + return $this->apiResponse->cachedJsonResponse($this->service->getSchemas()); } /** @@ -82,7 +78,7 @@ public function getSchemas() { */ public function getSchema(string $identifier) { try { - return $this->apiResponse->cachedJsonResponse($this->metastoreService->getSchema($identifier)); + return $this->apiResponse->cachedJsonResponse($this->service->getSchema($identifier)); } catch (\Exception $e) { return $this->getResponseFromException($e, 404); @@ -105,10 +101,10 @@ public function getAll(string $schema_id, Request $request) { $output = array_map(function ($object) use ($keepRefs) { $modified_object = $keepRefs - ? $this->metastoreService->swapReferences($object) - : $this->metastoreService->removeReferences($object); + ? $this->service->swapReferences($object) + : $this->service->removeReferences($object); return (object) $modified_object->get('$'); - }, $this->metastoreService->getAll($schema_id)); + }, $this->service->getAll($schema_id)); $output = array_values($output); return $this->apiResponse->cachedJsonResponse($output, 200, [$schema_id], $request->query); @@ -132,9 +128,9 @@ public function getAll(string $schema_id, Request $request) { */ public function get(string $schema_id, string $identifier, Request $request) { try { - $object = $this->metastoreService->get($schema_id, $identifier); + $object = $this->service->get($schema_id, $identifier); if ($this->wantObjectWithReferences($request)) { - $object = $this->metastoreService->swapReferences($object); + $object = $this->service->swapReferences($object); } else { $object = MetastoreService::removeReferences($object); @@ -180,8 +176,8 @@ public function post(string $schema_id, Request $request) { try { $data = $request->getContent(); $this->checkIdentifier($data); - $data = $this->metastoreService->getValidMetadataFactory()->get($data, $schema_id, ['method' => 'POST']); - $identifier = $this->metastoreService->post($schema_id, $data); + $data = $this->service->getValidMetadataFactory()->get($data, $schema_id, ['method' => 'POST']); + $identifier = $this->service->post($schema_id, $data); return $this->apiResponse->cachedJsonResponse([ "endpoint" => "{$request->getRequestUri()}/{$identifier}", "identifier" => $identifier, @@ -210,7 +206,7 @@ public function post(string $schema_id, Request $request) { */ public function publish(string $schema_id, string $identifier, Request $request) { try { - $this->metastoreService->publish($schema_id, $identifier); + $this->service->publish($schema_id, $identifier); return $this->apiResponse->cachedJsonResponse((object) [ "endpoint" => "{$request->getRequestUri()}/publish", "identifier" => $identifier, @@ -241,8 +237,8 @@ public function put($schema_id, string $identifier, Request $request) { try { $data = $request->getContent(); $this->checkIdentifier($data, $identifier); - $data = $this->metastoreService->getValidMetadataFactory()->get($data, $schema_id); - $info = $this->metastoreService->put($schema_id, $identifier, $data); + $data = $this->service->getValidMetadataFactory()->get($data, $schema_id); + $info = $this->service->put($schema_id, $identifier, $data); $code = ($info['new'] == TRUE) ? 201 : 200; return $this->apiResponse->cachedJsonResponse( [ @@ -287,7 +283,7 @@ public function patch($schema_id, $identifier, Request $request) { } $this->checkIdentifier($data, $identifier); - $this->metastoreService->patch($schema_id, $identifier, $data); + $this->service->patch($schema_id, $identifier, $data); return $this->apiResponse->cachedJsonResponse((object) [ "endpoint" => $request->getRequestUri(), "identifier" => $identifier, @@ -314,7 +310,7 @@ public function patch($schema_id, $identifier, Request $request) { */ public function delete($schema_id, $identifier) { try { - $this->metastoreService->delete($schema_id, $identifier); + $this->service->delete($schema_id, $identifier); return $this->apiResponse->cachedJsonResponse((object) ["message" => "Dataset {$identifier} has been deleted."]); } catch (\Exception $e) { @@ -330,7 +326,7 @@ public function delete($schema_id, $identifier) { */ public function getCatalog() : JsonResponse { try { - return $this->apiResponse->cachedJsonResponse($this->metastoreService->getCatalog()); + return $this->apiResponse->cachedJsonResponse($this->service->getCatalog()); } catch (\Exception $e) { return $this->getResponseFromException($e); diff --git a/modules/metastore/src/Reference/ReferenceLookup.php b/modules/metastore/src/Reference/ReferenceLookup.php index d0609fa352..831947409f 100644 --- a/modules/metastore/src/Reference/ReferenceLookup.php +++ b/modules/metastore/src/Reference/ReferenceLookup.php @@ -134,6 +134,9 @@ protected function decodeJsonMetadata(string $json): array { $module_path = $this->moduleHandler->getModule(get_module_name())->getPath(); $legacy_schema_path = $module_path . '/docs/legacy_metadata.json'; // Fetch the legacy metadata schema. + // @todo This file load happens for every metadata item that is processed. + // The schema JSON is then garbage-collected when we leave the scope of + // this function. Find a way to keep and reuse the schema data. $legacy_schema = file_get_contents($legacy_schema_path); // Record metadata identifier. $identifier = $metadata->identifier;