From 4cdc411eb632aa93c7011d63f72638083d0c64c9 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Mon, 18 Dec 2023 10:36:10 -0800 Subject: [PATCH 1/7] added ImportJobStoreFactory --- .../common/src/Storage/JobStoreFactory.php | 2 +- modules/datastore/datastore.services.yml | 5 +++ .../src/Storage/ImportJobStoreFactory.php | 45 +++++++++++++++++++ 3 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 modules/datastore/src/Storage/ImportJobStoreFactory.php diff --git a/modules/common/src/Storage/JobStoreFactory.php b/modules/common/src/Storage/JobStoreFactory.php index 5fab8fae71..ce16e6e9eb 100644 --- a/modules/common/src/Storage/JobStoreFactory.php +++ b/modules/common/src/Storage/JobStoreFactory.php @@ -14,7 +14,7 @@ class JobStoreFactory implements StorageFactoryInterface { * * @var \Drupal\Core\Database\Connection */ - private Connection $connection; + protected Connection $connection; /** * Constructor. diff --git a/modules/datastore/datastore.services.yml b/modules/datastore/datastore.services.yml index 89677b6c0b..ee6a90fc2f 100644 --- a/modules/datastore/datastore.services.yml +++ b/modules/datastore/datastore.services.yml @@ -111,6 +111,11 @@ services: - '@dkan.common.job_store' - '@dkan.datastore.import_info' + dkan.datastore.import_job_store_factory: + class: \Drupal\datastore\Storage\ImportJobFactory + arguments: + - '@database' + pdlt.converter.strptime_to_mysql: class: \PDLT\Converter arguments: diff --git a/modules/datastore/src/Storage/ImportJobStoreFactory.php b/modules/datastore/src/Storage/ImportJobStoreFactory.php new file mode 100644 index 0000000000..b443fc8c8a --- /dev/null +++ b/modules/datastore/src/Storage/ImportJobStoreFactory.php @@ -0,0 +1,45 @@ +getDeprecatedTableName($identifier); + // Figure out whether we need a separate deprecated table name. This will + // be used in JobStore::destruct() to clean up deprecated tables if they + // exist. + if ($table_name === $deprecated_table_name) { + $deprecated_table_name = ''; + } + return new JobStore($table_name, $this->connection, $deprecated_table_name); + } + +} From aa4bfec30d4dde048d2314eb4ab1a4124a6754e7 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Tue, 19 Dec 2023 09:51:12 -0800 Subject: [PATCH 2/7] uses AbstractJobStoreFactory --- .../src/Storage/AbstractJobStoreFactory.php | 69 ++++++++++++++++ .../DeprecatedJobStoreFactoryTrait.php | 81 +++++++++++++++++++ .../common/src/Storage/JobStoreFactory.php | 78 +++--------------- modules/datastore/datastore.services.yml | 6 +- .../datastore_mysql_import.services.yml | 2 +- .../src/Factory/MysqlImportFactory.php | 12 +-- modules/datastore/src/DatastoreService.php | 15 +++- .../EventSubscriber/DatastoreSubscriber.php | 23 +++++- .../datastore/src/Service/Factory/Import.php | 5 +- .../Service/Factory/ImportServiceFactory.php | 12 +-- modules/datastore/src/Service/Import.php | 6 +- .../datastore/src/Service/ImportService.php | 18 ++--- .../src/Storage/ImportJobStoreFactory.php | 40 ++------- .../QueueWorker/ImportQueueWorkerTest.php | 3 + .../src/Kernel/Service/ImportServiceTest.php | 2 +- .../tests/src/Unit/DatastoreServiceTest.php | 3 + .../DatastoreSubscriberTest.php | 8 ++ .../src/Unit/Service/DatastoreQueryTest.php | 2 + .../Factory/ImportServiceFactoryTest.php | 4 +- 19 files changed, 248 insertions(+), 141 deletions(-) create mode 100644 modules/common/src/Storage/AbstractJobStoreFactory.php create mode 100644 modules/common/src/Storage/DeprecatedJobStoreFactoryTrait.php diff --git a/modules/common/src/Storage/AbstractJobStoreFactory.php b/modules/common/src/Storage/AbstractJobStoreFactory.php new file mode 100644 index 0000000000..52522cc6aa --- /dev/null +++ b/modules/common/src/Storage/AbstractJobStoreFactory.php @@ -0,0 +1,69 @@ +connection = $connection; + } + + /** + * {@inheritDoc} + * + * @param string $identifier + * (optional) An identifier. This is optional because unless there is an + * existing table with a deprecated name, we'll use the table name from + * self::TABLE_NAME. + * @param array $config + * (optional) Ignored, because JobStore does not use it. + * + * @return \Drupal\common\Storage\DatabaseTableInterface + * Resulting JobStore object. + */ + public function getInstance(string $identifier = '', array $config = []): DatabaseTableInterface { + // For historical reasons, we keep the getInstance() method signature, but + // we also want to enforce our static table name. + if ($identifier && $identifier !== self::TABLE_NAME) { + // Silent error to be picked up by tests. + @trigger_error( + 'Import job store identifier must be either empty or ' . self::TABLE_NAME . '.', + E_USER_DEPRECATED + ); + } + $table_name = self::TABLE_NAME; + $deprecated_table_name = $this->getDeprecatedTableName($identifier); + // Figure out whether we need a separate deprecated table name. This will + // be used in JobStore::destruct() to clean up deprecated tables if they + // exist. + if ($table_name === $deprecated_table_name) { + $deprecated_table_name = ''; + } + return new JobStore($table_name, $this->connection, $deprecated_table_name); + } + +} diff --git a/modules/common/src/Storage/DeprecatedJobStoreFactoryTrait.php b/modules/common/src/Storage/DeprecatedJobStoreFactoryTrait.php new file mode 100644 index 0000000000..a9227a4f9a --- /dev/null +++ b/modules/common/src/Storage/DeprecatedJobStoreFactoryTrait.php @@ -0,0 +1,81 @@ +getDeprecatedTableName($identifier); + if ($this->connection->schema()->tableExists($deprecatedTableName)) { + return $deprecatedTableName; + } + return $this->getHashedTableName($identifier); + } + return 'jobstore_' . $identifier; + } + + /** + * Hash the class name identifier to generate a table name. + * + * This is the preferred strategy for new jobstore tables. + * + * @return string + * The hashed table name. + */ + protected function getHashedTableName(string $identifier): string { + // Avoid table-name-too-long errors by hashing the FQN of the identifier, + // assuming it's a class name. + $exploded_class = explode('\\', $identifier); + $table_name = strtolower(implode('_', [ + 'jobstore', + crc32($identifier), + array_pop($exploded_class), + ])); + return $table_name; + } + + /** + * Get the deprecated table name. + * + * The deprecated table name is generated by replacing \ with _ in + * fully-qualified class names. + * + * This style of jobstore table name is deprecated. See release notes for + * DKAN 2.16.5. + * + * @return string + * Deprecated table name. + * + * @see https://github.com/GetDKAN/dkan/releases/tag/2.16.5 + */ + protected function getDeprecatedTableName(string $identifier): string { + $safeClassName = strtolower(preg_replace( + '/\\\\/', '_', + $identifier + )); + return 'jobstore_' . $safeClassName; + } + +} diff --git a/modules/common/src/Storage/JobStoreFactory.php b/modules/common/src/Storage/JobStoreFactory.php index ce16e6e9eb..ac1cf2b65f 100644 --- a/modules/common/src/Storage/JobStoreFactory.php +++ b/modules/common/src/Storage/JobStoreFactory.php @@ -6,9 +6,18 @@ /** * DKAN JobStore Factory. + * + * @deprecated This class still exists in code to provide backwards + * compatibility. + * + * @todo Add FileFetcherJobStoreFactory as well. + * + * @see \Drupal\common\Storage\AbstractJobStoreFactory */ class JobStoreFactory implements StorageFactoryInterface { + use DeprecatedJobStoreFactoryTrait; + /** * Drupal database connection. * @@ -51,73 +60,4 @@ public function getInstance(string $identifier, array $config = []): DatabaseTab return new JobStore($table_name, $this->connection, $deprecated_table_name); } - /** - * Get the table name, preferring the deprecated one if it exists. - * - * If the identifier contains a backslash (\), we assume it's a class name and - * could be either the deprecated or new hashed-style table name. We try to - * find out if the deprecated one exists. If it does, we use its name. - * Otherwise, we use the hashed-style table name. - * - * If the identifier does not contain a backslash, then we assume it is a - * table-name-safe string, and prepend it with 'jobstore_'. - * - * @todo Phase out the use of the deprecated and hashed-class table names in - * favor of string literal identifiers. - */ - protected function getTableName(string $identifier): string { - // Check for either \ or class_exists() because not all class names will - // contain a backslash. - if ((strpos($identifier, '\\') !== FALSE) || class_exists($identifier)) { - $deprecatedTableName = $this->getDeprecatedTableName($identifier); - if ($this->connection->schema()->tableExists($deprecatedTableName)) { - return $deprecatedTableName; - } - return $this->getHashedTableName($identifier); - } - return 'jobstore_' . $identifier; - } - - /** - * Hash the class name identifier to generate a table name. - * - * This is the preferred strategy for new jobstore tables. - * - * @return string - * The hashed table name. - */ - protected function getHashedTableName(string $identifier): string { - // Avoid table-name-too-long errors by hashing the FQN of the identifier, - // assuming it's a class name. - $exploded_class = explode('\\', $identifier); - $table_name = strtolower(implode('_', [ - 'jobstore', - crc32($identifier), - array_pop($exploded_class), - ])); - return $table_name; - } - - /** - * Get the deprecated table name. - * - * The deprecated table name is generated by replacing \ with _ in - * fully-qualified class names. - * - * This style of jobstore table name is deprecated. See release notes for - * DKAN 2.16.5. - * - * @return string - * Deprecated table name. - * - * @see https://github.com/GetDKAN/dkan/releases/tag/2.16.5 - */ - protected function getDeprecatedTableName(string $identifier): string { - $safeClassName = strtolower(preg_replace( - '/\\\\/', '_', - $identifier - )); - return 'jobstore_' . $safeClassName; - } - } diff --git a/modules/datastore/datastore.services.yml b/modules/datastore/datastore.services.yml index ee6a90fc2f..30e6262eed 100644 --- a/modules/datastore/datastore.services.yml +++ b/modules/datastore/datastore.services.yml @@ -6,6 +6,7 @@ services: - '@dkan.datastore.service.factory.import' - '@queue' - '@dkan.common.job_store' + - '@dkan.datastore.import_job_store_factory' - '@dkan.datastore.service.resource_processor.dictionary_enforcer' dkan.datastore.service.post_import: @@ -54,7 +55,7 @@ services: dkan.datastore.service.factory.import: class: \Drupal\datastore\Service\Factory\ImportServiceFactory arguments: - - '@dkan.common.job_store' + - '@dkan.datastore.import_job_store_factory' - '@dkan.datastore.database_table_factory' dkan.datastore.logger_channel: @@ -94,6 +95,7 @@ services: - '@dkan.datastore.service' - '@dkan.datastore.service.resource_purger' - '@dkan.common.job_store' + - '@dkan.datastore.import_job_store_factory' tags: - { name: event_subscriber } @@ -112,7 +114,7 @@ services: - '@dkan.datastore.import_info' dkan.datastore.import_job_store_factory: - class: \Drupal\datastore\Storage\ImportJobFactory + class: \Drupal\datastore\Storage\ImportJobStoreFactory arguments: - '@database' diff --git a/modules/datastore/modules/datastore_mysql_import/datastore_mysql_import.services.yml b/modules/datastore/modules/datastore_mysql_import/datastore_mysql_import.services.yml index 2e96abfe54..bf6d0fcc73 100644 --- a/modules/datastore/modules/datastore_mysql_import/datastore_mysql_import.services.yml +++ b/modules/datastore/modules/datastore_mysql_import/datastore_mysql_import.services.yml @@ -2,7 +2,7 @@ services: dkan.datastore.service.factory.import: class: \Drupal\datastore_mysql_import\Factory\MysqlImportFactory arguments: - - '@dkan.common.job_store' + - '@dkan.datastore.import_job_store_factory' - '@dkan.datastore_mysql_import.database_table_factory' dkan.datastore_mysql_import.database_table_factory: diff --git a/modules/datastore/modules/datastore_mysql_import/src/Factory/MysqlImportFactory.php b/modules/datastore/modules/datastore_mysql_import/src/Factory/MysqlImportFactory.php index 3a0a8280d9..cfe5767d8a 100644 --- a/modules/datastore/modules/datastore_mysql_import/src/Factory/MysqlImportFactory.php +++ b/modules/datastore/modules/datastore_mysql_import/src/Factory/MysqlImportFactory.php @@ -2,9 +2,9 @@ namespace Drupal\datastore_mysql_import\Factory; -use Drupal\common\Storage\JobStoreFactory; use Drupal\datastore\Service\Factory\ImportFactoryInterface; use Drupal\datastore\Service\ImportService; +use Drupal\datastore\Storage\ImportJobStoreFactory; use Drupal\datastore_mysql_import\Service\MysqlImport; use Drupal\datastore_mysql_import\Storage\MySqlDatabaseTableFactory; @@ -16,9 +16,9 @@ class MysqlImportFactory implements ImportFactoryInterface { /** * The JobStore Factory service. * - * @var \Drupal\common\Storage\JobStoreFactory + * @var \Drupal\datastore\Storage\ImportJobStoreFactory */ - private $jobStoreFactory; + private ImportJobStoreFactory $importJobStoreFactory; /** * Database table factory service. @@ -30,8 +30,8 @@ class MysqlImportFactory implements ImportFactoryInterface { /** * Constructor. */ - public function __construct(JobStoreFactory $jobStoreFactory, MySqlDatabaseTableFactory $databaseTableFactory) { - $this->jobStoreFactory = $jobStoreFactory; + public function __construct(ImportJobStoreFactory $importJobStoreFactory, MySqlDatabaseTableFactory $databaseTableFactory) { + $this->importJobStoreFactory = $importJobStoreFactory; $this->databaseTableFactory = $databaseTableFactory; } @@ -46,7 +46,7 @@ public function getInstance(string $identifier, array $config = []) { throw new \Exception("config['resource'] is required"); } - $importer = new ImportService($resource, $this->jobStoreFactory, $this->databaseTableFactory); + $importer = new ImportService($resource, $this->importJobStoreFactory, $this->databaseTableFactory); $importer->setImporterClass(MysqlImport::class); return $importer; } diff --git a/modules/datastore/src/DatastoreService.php b/modules/datastore/src/DatastoreService.php index 625fa9e533..a44a0eae71 100644 --- a/modules/datastore/src/DatastoreService.php +++ b/modules/datastore/src/DatastoreService.php @@ -5,11 +5,11 @@ use Drupal\common\DataResource; use Drupal\common\Storage\JobStoreFactory; use Drupal\datastore\Service\ImportService; +use Drupal\datastore\Storage\ImportJobStoreFactory; use Procrastinator\Result; use Symfony\Component\DependencyInjection\ContainerInterface; use Drupal\Core\DependencyInjection\ContainerInjectionInterface; use Drupal\Core\Queue\QueueFactory; -use Drupal\datastore\Plugin\QueueWorker\ImportJob; use Drupal\datastore\Service\Factory\ImportFactoryInterface; use Drupal\datastore\Service\ResourceLocalizer; use Drupal\datastore\Service\ResourceProcessor\DictionaryEnforcer; @@ -55,6 +55,13 @@ class DatastoreService implements ContainerInjectionInterface { */ private $dictionaryEnforcer; + /** + * Import job store factory. + * + * @var \Drupal\datastore\Storage\ImportJobStoreFactory + */ + private ImportJobStoreFactory $importJobStoreFactory; + /** * {@inheritdoc} */ @@ -64,6 +71,7 @@ public static function create(ContainerInterface $container) { $container->get('dkan.datastore.service.factory.import'), $container->get('queue'), $container->get('dkan.common.job_store'), + $container->get('dkan.datastore.import_job_store_factory'), $container->get('dkan.datastore.service.resource_processor.dictionary_enforcer') ); } @@ -87,12 +95,14 @@ public function __construct( ImportFactoryInterface $importServiceFactory, QueueFactory $queue, JobStoreFactory $jobStoreFactory, + ImportJobStoreFactory $importJobStoreFactory, DictionaryEnforcer $dictionaryEnforcer ) { $this->queue = $queue; $this->resourceLocalizer = $resourceLocalizer; $this->importServiceFactory = $importServiceFactory; $this->jobStoreFactory = $jobStoreFactory; + $this->importJobStoreFactory = $importJobStoreFactory; $this->dictionaryEnforcer = $dictionaryEnforcer; } @@ -223,8 +233,7 @@ public function drop(string $identifier, ?string $version = NULL, bool $local_re if ($storage) { $storage->destruct(); - $this->jobStoreFactory - ->getInstance(ImportJob::class) + $this->importJobStoreFactory->getInstance() ->remove(md5($resource->getUniqueIdentifier())); } diff --git a/modules/datastore/src/EventSubscriber/DatastoreSubscriber.php b/modules/datastore/src/EventSubscriber/DatastoreSubscriber.php index 1067b1bc86..82037ae412 100644 --- a/modules/datastore/src/EventSubscriber/DatastoreSubscriber.php +++ b/modules/datastore/src/EventSubscriber/DatastoreSubscriber.php @@ -11,6 +11,7 @@ use Drupal\datastore\DatastoreService; use Drupal\datastore\Service\ResourceLocalizer; use Drupal\datastore\Service\ResourcePurger; +use Drupal\datastore\Storage\ImportJobStoreFactory; use Drupal\metastore\LifeCycle\LifeCycle; use Drupal\metastore\MetastoreItemInterface; use Drupal\metastore\ResourceMapper; @@ -38,6 +39,13 @@ class DatastoreSubscriber implements EventSubscriberInterface { */ protected $loggerFactory; + /** + * Import job store factory. + * + * @var \Drupal\datastore\Storage\ImportJobStoreFactory + */ + private ImportJobStoreFactory $importJobStoreFactory; + /** * Inherited. * @@ -49,7 +57,8 @@ public static function create(ContainerInterface $container) { $container->get('logger.factory'), $container->get('dkan.datastore.service'), $container->get('dkan.datastore.service.resource_purger'), - $container->get('dkan.common.job_store') + $container->get('dkan.common.job_store'), + $container->get('dkan.datastore.import_job_store_factory') ); } @@ -67,12 +76,20 @@ public static function create(ContainerInterface $container) { * @param \Drupal\common\Storage\JobStoreFactory $jobStoreFactory * The dkan.common.job_store service. */ - public function __construct(ConfigFactoryInterface $config_factory, LoggerChannelFactory $logger_factory, DatastoreService $service, ResourcePurger $resourcePurger, JobStoreFactory $jobStoreFactory) { + public function __construct( + ConfigFactoryInterface $config_factory, + LoggerChannelFactory $logger_factory, + DatastoreService $service, + ResourcePurger $resourcePurger, + JobStoreFactory $jobStoreFactory, + ImportJobStoreFactory $importJobStoreFactory + ) { $this->configFactory = $config_factory; $this->loggerFactory = $logger_factory; $this->service = $service; $this->resourcePurger = $resourcePurger; $this->jobStoreFactory = $jobStoreFactory; + $this->importJobStoreFactory = $importJobStoreFactory; } /** @@ -152,7 +169,7 @@ public function drop(Event $event) { ]); } try { - $this->jobStoreFactory->getInstance(ImportJob::class)->remove($id); + $this->importJobStoreFactory->getInstance()->remove($id); } catch (\Exception $e) { $this->loggerFactory->get('datastore')->error('Failed to remove importer job. @message', diff --git a/modules/datastore/src/Service/Factory/Import.php b/modules/datastore/src/Service/Factory/Import.php index 0c025fbbc9..f61c4f7104 100644 --- a/modules/datastore/src/Service/Factory/Import.php +++ b/modules/datastore/src/Service/Factory/Import.php @@ -4,6 +4,7 @@ use Drupal\datastore\Storage\DatabaseTableFactory; use Drupal\common\Storage\JobStoreFactory; +use Drupal\datastore\Storage\ImportJobStoreFactory; /** * Create an importer object for a given resource. @@ -16,8 +17,8 @@ class Import extends ImportServiceFactory { /** * Constructor. */ - public function __construct(JobStoreFactory $jobStoreFactory, DatabaseTableFactory $databaseTableFactory) { - parent::__construct($jobStoreFactory, $databaseTableFactory); + public function __construct(ImportJobStoreFactory $importJobStoreFactory, DatabaseTableFactory $databaseTableFactory) { + parent::__construct($importJobStoreFactory, $databaseTableFactory); @trigger_error(__NAMESPACE__ . '\Import is deprecated. Use \Drupal\datastore\Service\Factory\ImportServiceFactory instead.', E_USER_DEPRECATED); } diff --git a/modules/datastore/src/Service/Factory/ImportServiceFactory.php b/modules/datastore/src/Service/Factory/ImportServiceFactory.php index 0a54f9ff91..fe69c3fa4d 100644 --- a/modules/datastore/src/Service/Factory/ImportServiceFactory.php +++ b/modules/datastore/src/Service/Factory/ImportServiceFactory.php @@ -4,7 +4,7 @@ use Drupal\datastore\Storage\DatabaseTableFactory; use Drupal\datastore\Service\ImportService; -use Drupal\common\Storage\JobStoreFactory; +use Drupal\datastore\Storage\ImportJobStoreFactory; /** * Create an importer object for a given resource. @@ -14,9 +14,9 @@ class ImportServiceFactory implements ImportFactoryInterface { /** * Job store factory. * - * @var \Drupal\common\Storage\JobStoreFactory + * @var \Drupal\datastore\Storage\ImportJobStoreFactory */ - private $jobStoreFactory; + private ImportJobStoreFactory $importJobStoreFactory; /** * Database table factory. @@ -35,8 +35,8 @@ class ImportServiceFactory implements ImportFactoryInterface { /** * Constructor. */ - public function __construct(JobStoreFactory $jobStoreFactory, DatabaseTableFactory $databaseTableFactory) { - $this->jobStoreFactory = $jobStoreFactory; + public function __construct(ImportJobStoreFactory $importJobStoreFactory, DatabaseTableFactory $databaseTableFactory) { + $this->importJobStoreFactory = $importJobStoreFactory; $this->databaseTableFactory = $databaseTableFactory; } @@ -52,7 +52,7 @@ public function getInstance(string $identifier, array $config = []) { } $resource = $config['resource']; - return new ImportService($resource, $this->jobStoreFactory, $this->databaseTableFactory); + return new ImportService($resource, $this->importJobStoreFactory, $this->databaseTableFactory); } } diff --git a/modules/datastore/src/Service/Import.php b/modules/datastore/src/Service/Import.php index 09a5dd7ba9..498e432e31 100644 --- a/modules/datastore/src/Service/Import.php +++ b/modules/datastore/src/Service/Import.php @@ -19,13 +19,13 @@ class Import extends ImportService { * * @param \Drupal\common\DataResource $resource * DKAN Resource. - * @param \Drupal\common\Storage\JobStoreFactory $jobStoreFactory + * @param \Drupal\common\Storage\JobStoreFactory $importJobStoreFactory * Jobstore factory. * @param \Drupal\datastore\Storage\DatabaseTableFactory $databaseTableFactory * Database Table factory. */ - public function __construct(DataResource $resource, JobStoreFactory $jobStoreFactory, DatabaseTableFactory $databaseTableFactory) { - parent::__construct($resource, $jobStoreFactory, $databaseTableFactory); + public function __construct(DataResource $resource, JobStoreFactory $importJobStoreFactory, DatabaseTableFactory $databaseTableFactory) { + parent::__construct($resource, $importJobStoreFactory, $databaseTableFactory); @trigger_error(__NAMESPACE__ . '\Import is deprecated. Use \Drupal\datastore\Service\ImportService instead.', E_USER_DEPRECATED); } diff --git a/modules/datastore/src/Service/ImportService.php b/modules/datastore/src/Service/ImportService.php index bf4cbf9ec2..8f6778737d 100644 --- a/modules/datastore/src/Service/ImportService.php +++ b/modules/datastore/src/Service/ImportService.php @@ -7,9 +7,9 @@ use Drupal\common\EventDispatcherTrait; use Drupal\common\LoggerTrait; use Drupal\common\DataResource; -use Drupal\common\Storage\JobStoreFactory; use Drupal\datastore\Storage\DatabaseTable; use Drupal\datastore\Storage\DatabaseTableFactory; +use Drupal\datastore\Storage\ImportJobStoreFactory; use Procrastinator\Result; /** @@ -54,11 +54,9 @@ class ImportService { /** * The jobstore factory service. * - * @var \Drupal\common\Storage\JobStoreFactory - * - * @todo Can we remove this? + * @var \Drupal\datastore\Storage\ImportJobStoreFactory */ - private JobStoreFactory $jobStoreFactory; + private ImportJobStoreFactory $importJobStoreFactory; /** * Database table factory service. @@ -83,14 +81,14 @@ class ImportService { * * @param \Drupal\common\DataResource $resource * DKAN Resource. - * @param \Drupal\common\Storage\JobStoreFactory $jobStoreFactory - * Jobstore factory. + * @param \Drupal\datastore\Storage\ImportJobStoreFactory $importJobStoreFactory + * Import jobstore factory. * @param \Drupal\datastore\Storage\DatabaseTableFactory $databaseTableFactory * Database Table factory. */ - public function __construct(DataResource $resource, JobStoreFactory $jobStoreFactory, DatabaseTableFactory $databaseTableFactory) { + public function __construct(DataResource $resource, ImportJobStoreFactory $importJobStoreFactory, DatabaseTableFactory $databaseTableFactory) { $this->resource = $resource; - $this->jobStoreFactory = $jobStoreFactory; + $this->importJobStoreFactory = $importJobStoreFactory; $this->databaseTableFactory = $databaseTableFactory; } @@ -158,7 +156,7 @@ public function getImporter(): ImportJob { $this->importJob = call_user_func([$this->importerClass, 'get'], $datastore_resource->getId(), - $this->jobStoreFactory->getInstance(ImportJob::class), + $this->importJobStoreFactory->getInstance(), [ "storage" => $this->getStorage(), "parser" => $this->getNonRecordingParser($delimiter), diff --git a/modules/datastore/src/Storage/ImportJobStoreFactory.php b/modules/datastore/src/Storage/ImportJobStoreFactory.php index b443fc8c8a..126e6b222f 100644 --- a/modules/datastore/src/Storage/ImportJobStoreFactory.php +++ b/modules/datastore/src/Storage/ImportJobStoreFactory.php @@ -2,44 +2,18 @@ namespace Drupal\datastore\Storage; -use Drupal\common\Storage\DatabaseTableInterface; -use Drupal\common\Storage\JobStore; -use Drupal\common\Storage\JobStoreFactory; +use Drupal\common\Storage\AbstractJobStoreFactory; -class ImportJobStoreFactory extends JobStoreFactory { - - protected const TABLE_NAME = 'jobstore_2613055649_importjob'; +/** + * Create a job store object for the import process. + */ +class ImportJobStoreFactory extends AbstractJobStoreFactory { /** * {@inheritDoc} * - * @param string $identifier - * (optional) An identifier. This is optional because unless there is an - * existing table with a deprecated name, we'll use the table name from - * self::TABLE_NAME. - * @param array $config - * (optional) Ignored, because JobStore does not use it. - * - * @return \Drupal\common\Storage\DatabaseTableInterface - * Resulting JobStore object. + * This string contains an ugly hash for historical reasons. */ - public function getInstance(string $identifier = '', array $config = []): DatabaseTableInterface { - if ($identifier && $identifier !== self::TABLE_NAME) { - // Silent error to be picked up by tests. - @trigger_error( - 'Import job store identifier must be either empty or ' . self::TABLE_NAME . '.', - E_USER_DEPRECATED - ); - } - $table_name = self::TABLE_NAME; - $deprecated_table_name = $this->getDeprecatedTableName($identifier); - // Figure out whether we need a separate deprecated table name. This will - // be used in JobStore::destruct() to clean up deprecated tables if they - // exist. - if ($table_name === $deprecated_table_name) { - $deprecated_table_name = ''; - } - return new JobStore($table_name, $this->connection, $deprecated_table_name); - } + protected const TABLE_NAME = 'jobstore_2613055649_importjob'; } diff --git a/modules/datastore/tests/src/Kernel/Plugin/QueueWorker/ImportQueueWorkerTest.php b/modules/datastore/tests/src/Kernel/Plugin/QueueWorker/ImportQueueWorkerTest.php index a5917e8247..adc0a3262b 100644 --- a/modules/datastore/tests/src/Kernel/Plugin/QueueWorker/ImportQueueWorkerTest.php +++ b/modules/datastore/tests/src/Kernel/Plugin/QueueWorker/ImportQueueWorkerTest.php @@ -42,6 +42,7 @@ public function testErrorPath() { $this->container->get('dkan.datastore.service.factory.import'), $this->container->get('queue'), $this->container->get('dkan.common.job_store'), + $this->container->get('dkan.datastore.import_job_store_factory'), $this->container->get('dkan.datastore.service.resource_processor.dictionary_enforcer'), ]) ->onlyMethods(['import']) @@ -90,6 +91,7 @@ public function testRequeue() { $this->container->get('dkan.datastore.service.factory.import'), $this->container->get('queue'), $this->container->get('dkan.common.job_store'), + $this->container->get('dkan.datastore.import_job_store_factory'), $this->container->get('dkan.datastore.service.resource_processor.dictionary_enforcer'), ]) ->onlyMethods(['import']) @@ -228,6 +230,7 @@ public function testAlreadyImported() { $this->container->get('dkan.datastore.service.factory.import'), $this->container->get('queue'), $this->container->get('dkan.common.job_store'), + $this->container->get('dkan.datastore.import_job_store_factory'), $this->container->get('dkan.datastore.service.resource_processor.dictionary_enforcer'), ]) ->onlyMethods(['getStorage']) diff --git a/modules/datastore/tests/src/Kernel/Service/ImportServiceTest.php b/modules/datastore/tests/src/Kernel/Service/ImportServiceTest.php index 9d0311303c..d588705026 100644 --- a/modules/datastore/tests/src/Kernel/Service/ImportServiceTest.php +++ b/modules/datastore/tests/src/Kernel/Service/ImportServiceTest.php @@ -50,7 +50,7 @@ public function testImport() { ->onlyMethods(['getImporter']) ->setConstructorArgs([ new DataResource('abc.txt', 'text/csv'), - $this->container->get('dkan.common.job_store'), + $this->container->get('dkan.datastore.import_job_store_factory'), $this->container->get('dkan.datastore.database_table_factory'), ]) ->getMock(); diff --git a/modules/datastore/tests/src/Unit/DatastoreServiceTest.php b/modules/datastore/tests/src/Unit/DatastoreServiceTest.php index 9b4bba21d2..6e3dab7f4e 100644 --- a/modules/datastore/tests/src/Unit/DatastoreServiceTest.php +++ b/modules/datastore/tests/src/Unit/DatastoreServiceTest.php @@ -5,6 +5,7 @@ use Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher; use Drupal\Core\Queue\QueueFactory; use Drupal\datastore\Plugin\QueueWorker\ImportJob; +use Drupal\datastore\Storage\ImportJobStoreFactory; use Drupal\Tests\common\Traits\ServiceCheckTrait; use Drupal\common\DataResource; use Drupal\common\Storage\JobStore; @@ -63,6 +64,7 @@ public function testDrop() { ->add(DatabaseTable::class, 'destruct') ->add(ResourceLocalizer::class, 'remove') ->add(JobStoreFactory::class, 'getInstance', JobStore::class) + ->add(ImportJobStoreFactory::class, 'getInstance', JobStore::class) ->add(JobStore::class, 'remove', TRUE); $service = DatastoreService::create($mockChain->getMock()); @@ -96,6 +98,7 @@ private function getCommonChain() { ->add('dkan.datastore.service.factory.import', ImportServiceFactory::class) ->add('queue', QueueFactory::class) ->add('dkan.common.job_store', JobStoreFactory::class) + ->add('dkan.datastore.import_job_store_factory', ImportJobStoreFactory::class) ->add('dkan.datastore.import_info_list', ImportInfoList::class) ->add('dkan.datastore.service.resource_processor.dictionary_enforcer', DictionaryEnforcer::class) ->index(0); diff --git a/modules/datastore/tests/src/Unit/EventSubscriber/DatastoreSubscriberTest.php b/modules/datastore/tests/src/Unit/EventSubscriber/DatastoreSubscriberTest.php index e141aefd04..8375b1956d 100644 --- a/modules/datastore/tests/src/Unit/EventSubscriber/DatastoreSubscriberTest.php +++ b/modules/datastore/tests/src/Unit/EventSubscriber/DatastoreSubscriberTest.php @@ -14,6 +14,7 @@ use Drupal\datastore\Service\ResourcePurger; use Drupal\datastore\Storage\DatabaseTable; use Drupal\common\Events\Event; +use Drupal\datastore\Storage\ImportJobStoreFactory; use MockChain\Chain; use MockChain\Options; use PHPUnit\Framework\TestCase; @@ -103,6 +104,7 @@ public function testDrop() { ->add('dkan.datastore.service', DatastoreService::class) ->add('dkan.datastore.service.resource_purger', ResourcePurger::class) ->add('dkan.common.job_store', JobStoreFactory::class) + ->add('dkan.datastore.import_job_store_factory', ImportJobStoreFactory::class) ->add("database", Connection::class) ->index(0); @@ -113,6 +115,7 @@ public function testDrop() { ->add(ImportServiceFactory::class, 'getInstance', ImportService::class) ->add(ImportService::class, 'remove') ->add(JobStoreFactory::class, 'getInstance', JobStore::class) + ->add(ImportJobStoreFactory::class, 'getInstance', JobStore::class) ->add(JobStore::class, 'remove') ->add(LoggerChannelFactory::class, 'get', LoggerChannelInterface::class) ->add(LoggerChannelInterface::class, 'error', NULL, 'errors') @@ -138,6 +141,7 @@ public function testDatastoreDropException() { ->add('dkan.datastore.service', DatastoreService::class) ->add('dkan.datastore.service.resource_purger', ResourcePurger::class) ->add('dkan.common.job_store', JobStoreFactory::class) + ->add('dkan.datastore.import_job_store_factory', ImportJobStoreFactory::class) ->add("database", Connection::class) ->index(0); @@ -147,6 +151,7 @@ public function testDatastoreDropException() { ->add(ImportServiceFactory::class, 'getInstance', ImportService::class) ->add(ImportService::class, 'remove') ->add(JobStoreFactory::class, 'getInstance', JobStore::class) + ->add(ImportJobStoreFactory::class, 'getInstance', JobStore::class) ->add(JobStore::class, 'remove') ->add(LoggerChannelFactory::class, 'get', LoggerChannelInterface::class) ->add(LoggerChannelInterface::class, 'error', NULL, 'errors') @@ -171,6 +176,7 @@ public function testJobStoreRemoveException() { ->add('dkan.datastore.service', DatastoreService::class) ->add('dkan.datastore.service.resource_purger', ResourcePurger::class) ->add('dkan.common.job_store', JobStoreFactory::class) + ->add('dkan.datastore.import_job_store_factory', ImportJobStoreFactory::class) ->add("database", Connection::class) ->index(0); @@ -180,6 +186,7 @@ public function testJobStoreRemoveException() { ->add(ImportServiceFactory::class, 'getInstance', ImportService::class) ->add(ImportService::class, 'remove') ->add(JobStoreFactory::class, 'getInstance', JobStore::class) + ->add(ImportJobStoreFactory::class, 'getInstance', JobStore::class) ->add(JobStore::class, 'remove', new \Exception('error')) ->add(LoggerChannelFactory::class, 'get', LoggerChannelInterface::class) ->add(LoggerChannelInterface::class, 'error', NULL, 'errors') @@ -200,6 +207,7 @@ private function getContainerChain() { ->add('dkan.datastore.service', DatastoreService::class) ->add('dkan.datastore.service.resource_purger', ResourcePurger::class) ->add('dkan.common.job_store', JobStoreFactory::class) + ->add('dkan.datastore.import_job_store_factory', ImportJobStoreFactory::class) ->index(0); return (new Chain($this)) diff --git a/modules/datastore/tests/src/Unit/Service/DatastoreQueryTest.php b/modules/datastore/tests/src/Unit/Service/DatastoreQueryTest.php index a2d93f076e..cc49b89f0e 100644 --- a/modules/datastore/tests/src/Unit/Service/DatastoreQueryTest.php +++ b/modules/datastore/tests/src/Unit/Service/DatastoreQueryTest.php @@ -6,6 +6,7 @@ use Drupal\Core\DependencyInjection\Container; use Drupal\common\Storage\JobStoreFactory; use Drupal\Core\Queue\QueueFactory; +use Drupal\datastore\Storage\ImportJobStoreFactory; use Drupal\Tests\datastore\Traits\TestHelperTrait; use MockChain\Chain; use MockChain\Options; @@ -194,6 +195,7 @@ public function getCommonMockChain() { ->add('queue', QueueFactory::class) ->add('request_stack', RequestStack::class) ->add('dkan.common.job_store', JobStoreFactory::class) + ->add('dkan.datastore.import_job_store_factory', ImportJobStoreFactory::class) ->add('dkan.metastore.storage', DataFactory::class) ->add('dkan.datastore.import_info_list', ImportInfoList::class) ->add('dkan.datastore.service.resource_processor.dictionary_enforcer', DictionaryEnforcer::class) diff --git a/modules/datastore/tests/src/Unit/Service/Factory/ImportServiceFactoryTest.php b/modules/datastore/tests/src/Unit/Service/Factory/ImportServiceFactoryTest.php index f898d72e55..c63c924545 100644 --- a/modules/datastore/tests/src/Unit/Service/Factory/ImportServiceFactoryTest.php +++ b/modules/datastore/tests/src/Unit/Service/Factory/ImportServiceFactoryTest.php @@ -2,9 +2,9 @@ namespace Drupal\Tests\datastore\Unit\Service\Factory; -use Drupal\common\Storage\JobStoreFactory; use Drupal\datastore\Storage\DatabaseTableFactory; use Drupal\datastore\Service\Factory\ImportServiceFactory; +use Drupal\datastore\Storage\ImportJobStoreFactory; use PHPUnit\Framework\TestCase; /** @@ -21,7 +21,7 @@ class ImportServiceFactoryTest extends TestCase { */ public function testGetInstanceException() { $factory = new ImportServiceFactory( - $this->getMockBuilder(JobStoreFactory::class) + $this->getMockBuilder(ImportJobStoreFactory::class) ->disableOriginalConstructor() ->getMock(), $this->getMockBuilder(DatabaseTableFactory::class) From d689bc810b31d94041ddc753adde0685b3565379 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Fri, 22 Dec 2023 13:24:19 -0800 Subject: [PATCH 3/7] not a const --- modules/common/src/Storage/AbstractJobStoreFactory.php | 10 +++++----- .../datastore/src/Storage/ImportJobStoreFactory.php | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/modules/common/src/Storage/AbstractJobStoreFactory.php b/modules/common/src/Storage/AbstractJobStoreFactory.php index 52522cc6aa..3751f66edb 100644 --- a/modules/common/src/Storage/AbstractJobStoreFactory.php +++ b/modules/common/src/Storage/AbstractJobStoreFactory.php @@ -16,7 +16,7 @@ abstract class AbstractJobStoreFactory implements StorageFactoryInterface { * * Override this for your table name. */ - protected const TABLE_NAME = ''; + protected string $TABLE_NAME = ''; /** * Drupal database connection. @@ -38,7 +38,7 @@ public function __construct(Connection $connection) { * @param string $identifier * (optional) An identifier. This is optional because unless there is an * existing table with a deprecated name, we'll use the table name from - * self::TABLE_NAME. + * $this->TABLE_NAME. * @param array $config * (optional) Ignored, because JobStore does not use it. * @@ -48,14 +48,14 @@ public function __construct(Connection $connection) { public function getInstance(string $identifier = '', array $config = []): DatabaseTableInterface { // For historical reasons, we keep the getInstance() method signature, but // we also want to enforce our static table name. - if ($identifier && $identifier !== self::TABLE_NAME) { + if ($identifier && $identifier !== $this->TABLE_NAME) { // Silent error to be picked up by tests. @trigger_error( - 'Import job store identifier must be either empty or ' . self::TABLE_NAME . '.', + 'Import job store identifier must be either empty or ' . $this->TABLE_NAME . '.', E_USER_DEPRECATED ); } - $table_name = self::TABLE_NAME; + $table_name = $this->TABLE_NAME; $deprecated_table_name = $this->getDeprecatedTableName($identifier); // Figure out whether we need a separate deprecated table name. This will // be used in JobStore::destruct() to clean up deprecated tables if they diff --git a/modules/datastore/src/Storage/ImportJobStoreFactory.php b/modules/datastore/src/Storage/ImportJobStoreFactory.php index 126e6b222f..3bc26c3ee6 100644 --- a/modules/datastore/src/Storage/ImportJobStoreFactory.php +++ b/modules/datastore/src/Storage/ImportJobStoreFactory.php @@ -14,6 +14,6 @@ class ImportJobStoreFactory extends AbstractJobStoreFactory { * * This string contains an ugly hash for historical reasons. */ - protected const TABLE_NAME = 'jobstore_2613055649_importjob'; + protected string $TABLE_NAME = 'jobstore_2613055649_importjob'; } From da7bed72296a32cf080ac27a5eee9e5e06f0d62b Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Fri, 22 Dec 2023 15:56:22 -0800 Subject: [PATCH 4/7] beginning test --- .../src/Storage/AbstractJobStoreFactory.php | 8 +- .../Storage/AbstractJobStoreFactoryTest.php | 145 ++++++++++++++++++ .../Kernel/Storage/JobStoreFactoryTest.php | 4 +- .../src/Storage/ImportJobStoreFactory.php | 2 +- 4 files changed, 152 insertions(+), 7 deletions(-) create mode 100644 modules/common/tests/src/Kernel/Storage/AbstractJobStoreFactoryTest.php diff --git a/modules/common/src/Storage/AbstractJobStoreFactory.php b/modules/common/src/Storage/AbstractJobStoreFactory.php index 3751f66edb..534b785035 100644 --- a/modules/common/src/Storage/AbstractJobStoreFactory.php +++ b/modules/common/src/Storage/AbstractJobStoreFactory.php @@ -16,7 +16,7 @@ abstract class AbstractJobStoreFactory implements StorageFactoryInterface { * * Override this for your table name. */ - protected string $TABLE_NAME = ''; + protected string $tableName = ''; /** * Drupal database connection. @@ -48,14 +48,14 @@ public function __construct(Connection $connection) { public function getInstance(string $identifier = '', array $config = []): DatabaseTableInterface { // For historical reasons, we keep the getInstance() method signature, but // we also want to enforce our static table name. - if ($identifier && $identifier !== $this->TABLE_NAME) { + if ($identifier && $identifier !== $this->tableName) { // Silent error to be picked up by tests. @trigger_error( - 'Import job store identifier must be either empty or ' . $this->TABLE_NAME . '.', + 'Import job store identifier must be either empty or ' . $this->tableName . '.', E_USER_DEPRECATED ); } - $table_name = $this->TABLE_NAME; + $table_name = $this->tableName; $deprecated_table_name = $this->getDeprecatedTableName($identifier); // Figure out whether we need a separate deprecated table name. This will // be used in JobStore::destruct() to clean up deprecated tables if they diff --git a/modules/common/tests/src/Kernel/Storage/AbstractJobStoreFactoryTest.php b/modules/common/tests/src/Kernel/Storage/AbstractJobStoreFactoryTest.php new file mode 100644 index 0000000000..f7b68fab4c --- /dev/null +++ b/modules/common/tests/src/Kernel/Storage/AbstractJobStoreFactoryTest.php @@ -0,0 +1,145 @@ +container->get('database'); + // Make a concrete AbstractJobStoreFactory object. + /** @var \DkanTestConcreteAbstractJobStoreFactory $job_store_factory */ + $job_store_factory = new \DkanTestConcreteAbstractJobStoreFactory($db); + $job_store = $job_store_factory->getInstance(\DkanTestConcreteJobSubclass::class); + + $this->assertInstanceOf(JobStore::class, $job_store); + + // First, get the table name without deprecation. + $ref_get_table_name = new \ReflectionMethod($job_store, 'getTableName'); + $ref_get_table_name->setAccessible(TRUE); + $table_name = $ref_get_table_name->invoke($job_store); + $this->assertEquals('concrete_job_store', $table_name); + + // This table does not exist. + $this->assertFalse( + $db->schema()->tableExists($table_name) + ); + + // Use the deprecated table name. + $ref_get_deprecated_table_name = new \ReflectionMethod($job_store_factory, 'getDeprecatedTableName'); + $ref_get_deprecated_table_name->setAccessible(TRUE); + $deprecated_table_name = $ref_get_deprecated_table_name->invokeArgs( + $job_store_factory, + [\DkanTestConcreteJobSubclass::class] + ); + $this->assertEquals('jobstore_dkantestconcretejobsubclass', $deprecated_table_name); + $ref_table_name = new \ReflectionProperty($job_store, 'tableName'); + $ref_table_name->setAccessible(TRUE); + $ref_table_name->setValue($job_store, $deprecated_table_name); + + // Write the table. Count() will create the table before counting rows. + $this->assertEquals(0, $job_store->count()); + // Assert that the deprecated table exists. + $this->assertTrue( + $db->schema()->tableExists($deprecated_table_name) + ); + // Assert that the non-deprecated table does not exist. + $this->assertFalse( + $db->schema()->tableExists($table_name) + ); + + // Create a new JobStore object. The factory should see that the + // deprecated table already exists and try to use it as the table name. + $job_store = $job_store_factory->getInstance(\DkanTestConcreteJobSubclass::class); + $this->assertEquals( + $deprecated_table_name, + $ref_get_table_name->invoke($job_store) + ); + + // Remove the table and create yet another job store object. This + // one should have the non-deprecated table name. + $job_store->destruct(); + $this->assertFalse( + $db->schema()->tableExists($deprecated_table_name) + ); + $this->assertFalse( + $db->schema()->tableExists($table_name) + ); + $job_store = $job_store_factory->getInstance(\DkanTestConcreteJobSubclass::class); + $this->assertEquals($table_name, $ref_get_table_name->invoke($job_store)); + $this->assertEquals(0, $job_store->count()); + $this->assertTrue( + $db->schema()->tableExists($table_name) + ); + + // Use an identifier string that contains backslash, but is not an + // existing class. + $job_store->destruct(); + $job_store = $job_store_factory->getInstance('test\\thisshouldneverbeaclassname'); + $this->assertEquals( + 'jobstore_2087357147_thisshouldneverbeaclassname', + $table_name = $ref_get_table_name->invoke($job_store) + ); + $this->assertEquals(0, $job_store->count()); + $this->assertTrue( + $db->schema()->tableExists($table_name) + ); + + // And finally, use an identifier string that doesn't look like a class + // name. + $identifier = 'testthisshouldneverbeaclassname'; + $job_store->destruct(); + $job_store = $job_store_factory->getInstance($identifier); + $this->assertEquals( + 'jobstore_' . $identifier, + $table_name = $ref_get_table_name->invoke($job_store) + ); + $this->assertEquals(0, $job_store->count()); + $this->assertTrue( + $db->schema()->tableExists($table_name) + ); + } + + } +} + +/** + * We must manage namespaces so that we don't end up with a too-long table name. + */ + +namespace { + + use Drupal\common\Storage\AbstractJobStoreFactory; + use Procrastinator\Job\Job; + + class DkanTestConcreteAbstractJobStoreFactory extends AbstractJobStoreFactory { + + protected string $tableName = 'concrete_job_store'; + + } + + class DkanTestConcreteJobSubclass extends Job { + + protected function runIt() { + } + + } +} diff --git a/modules/common/tests/src/Kernel/Storage/JobStoreFactoryTest.php b/modules/common/tests/src/Kernel/Storage/JobStoreFactoryTest.php index 4f0778f499..cbb9c2a081 100644 --- a/modules/common/tests/src/Kernel/Storage/JobStoreFactoryTest.php +++ b/modules/common/tests/src/Kernel/Storage/JobStoreFactoryTest.php @@ -10,8 +10,8 @@ use Drupal\KernelTests\KernelTestBase; /** - * @covers \Drupal\common\Storage\JobStore - * @coversDefaultClass \Drupal\common\Storage\JobStore + * @covers \Drupal\common\Storage\JobStoreFactory + * @coversDefaultClass \Drupal\common\Storage\JobStoreFactory * * @group dkan * @group common diff --git a/modules/datastore/src/Storage/ImportJobStoreFactory.php b/modules/datastore/src/Storage/ImportJobStoreFactory.php index 3bc26c3ee6..318708f23a 100644 --- a/modules/datastore/src/Storage/ImportJobStoreFactory.php +++ b/modules/datastore/src/Storage/ImportJobStoreFactory.php @@ -14,6 +14,6 @@ class ImportJobStoreFactory extends AbstractJobStoreFactory { * * This string contains an ugly hash for historical reasons. */ - protected string $TABLE_NAME = 'jobstore_2613055649_importjob'; + protected string $tableName = 'jobstore_2613055649_importjob'; } From 8ba6267afb26d3e71656ff28625a3a767f2785cb Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Tue, 26 Dec 2023 10:05:15 -0800 Subject: [PATCH 5/7] adds tests --- .../src/Storage/AbstractJobStoreFactory.php | 10 ++++---- .../Storage/AbstractJobStoreFactoryTest.php | 23 +++++++++++++++---- .../Kernel/Storage/JobStoreFactoryTest.php | 2 ++ 3 files changed, 25 insertions(+), 10 deletions(-) diff --git a/modules/common/src/Storage/AbstractJobStoreFactory.php b/modules/common/src/Storage/AbstractJobStoreFactory.php index 534b785035..587c6719f5 100644 --- a/modules/common/src/Storage/AbstractJobStoreFactory.php +++ b/modules/common/src/Storage/AbstractJobStoreFactory.php @@ -46,16 +46,14 @@ public function __construct(Connection $connection) { * Resulting JobStore object. */ public function getInstance(string $identifier = '', array $config = []): DatabaseTableInterface { + $table_name = $this->tableName; // For historical reasons, we keep the getInstance() method signature, but // we also want to enforce our static table name. if ($identifier && $identifier !== $this->tableName) { - // Silent error to be picked up by tests. - @trigger_error( - 'Import job store identifier must be either empty or ' . $this->tableName . '.', - E_USER_DEPRECATED - ); + // If the caller passed an unusual identifier, we should try to use the + // table name they desire for backwards compatibility. + $table_name = $this->getTableName($identifier); } - $table_name = $this->tableName; $deprecated_table_name = $this->getDeprecatedTableName($identifier); // Figure out whether we need a separate deprecated table name. This will // be used in JobStore::destruct() to clean up deprecated tables if they diff --git a/modules/common/tests/src/Kernel/Storage/AbstractJobStoreFactoryTest.php b/modules/common/tests/src/Kernel/Storage/AbstractJobStoreFactoryTest.php index f7b68fab4c..21e8058811 100644 --- a/modules/common/tests/src/Kernel/Storage/AbstractJobStoreFactoryTest.php +++ b/modules/common/tests/src/Kernel/Storage/AbstractJobStoreFactoryTest.php @@ -16,6 +16,8 @@ * @group dkan * @group common * @group kernel + * + * @see \Drupal\Tests\common\Kernel\Storage\JobStoreFactoryTest */ class AbstractJobStoreFactoryTest extends KernelTestBase { @@ -28,15 +30,16 @@ public function testDeprecatedClassnameTable() { // Make a concrete AbstractJobStoreFactory object. /** @var \DkanTestConcreteAbstractJobStoreFactory $job_store_factory */ $job_store_factory = new \DkanTestConcreteAbstractJobStoreFactory($db); + // Get a Job object by specifying an instance name. $job_store = $job_store_factory->getInstance(\DkanTestConcreteJobSubclass::class); $this->assertInstanceOf(JobStore::class, $job_store); - // First, get the table name without deprecation. + // First, get the table name without deprecation by calculating a hash. $ref_get_table_name = new \ReflectionMethod($job_store, 'getTableName'); $ref_get_table_name->setAccessible(TRUE); $table_name = $ref_get_table_name->invoke($job_store); - $this->assertEquals('concrete_job_store', $table_name); + $this->assertEquals('jobstore_580088250_dkantestconcretejobsubclass', $table_name); // This table does not exist. $this->assertFalse( @@ -103,8 +106,7 @@ public function testDeprecatedClassnameTable() { $db->schema()->tableExists($table_name) ); - // And finally, use an identifier string that doesn't look like a class - // name. + // Use an identifier string that doesn't look like a class name. $identifier = 'testthisshouldneverbeaclassname'; $job_store->destruct(); $job_store = $job_store_factory->getInstance($identifier); @@ -116,6 +118,19 @@ public function testDeprecatedClassnameTable() { $this->assertTrue( $db->schema()->tableExists($table_name) ); + + // And finally, the right way. We create an instance without specifying + // an identifier, so that the factory uses its default. + $job_store->destruct(); + $job_store = $job_store_factory->getInstance(); + $this->assertEquals( + 'concrete_job_store', + $table_name = $ref_get_table_name->invoke($job_store) + ); + $this->assertEquals(0, $job_store->count()); + $this->assertTrue( + $db->schema()->tableExists($table_name) + ); } } diff --git a/modules/common/tests/src/Kernel/Storage/JobStoreFactoryTest.php b/modules/common/tests/src/Kernel/Storage/JobStoreFactoryTest.php index cbb9c2a081..3a7bb1b620 100644 --- a/modules/common/tests/src/Kernel/Storage/JobStoreFactoryTest.php +++ b/modules/common/tests/src/Kernel/Storage/JobStoreFactoryTest.php @@ -16,6 +16,8 @@ * @group dkan * @group common * @group kernel + * + * @see @see \Drupal\Tests\common\Kernel\Storage\AbstractJobStoreFactoryTest */ class JobStoreFactoryTest extends KernelTestBase { From 18d967b7608e332e725a264d43eb130e221d873c Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Tue, 26 Dec 2023 10:13:47 -0800 Subject: [PATCH 6/7] cs --- modules/common/src/Storage/AbstractJobStoreFactory.php | 2 ++ .../src/EventSubscriber/DatastoreSubscriber.php | 9 ++++----- modules/datastore/src/Service/Import.php | 8 ++++---- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/modules/common/src/Storage/AbstractJobStoreFactory.php b/modules/common/src/Storage/AbstractJobStoreFactory.php index 587c6719f5..5a3f043a6c 100644 --- a/modules/common/src/Storage/AbstractJobStoreFactory.php +++ b/modules/common/src/Storage/AbstractJobStoreFactory.php @@ -15,6 +15,8 @@ abstract class AbstractJobStoreFactory implements StorageFactoryInterface { * The import job store table name. * * Override this for your table name. + * + * @var string */ protected string $tableName = ''; diff --git a/modules/datastore/src/EventSubscriber/DatastoreSubscriber.php b/modules/datastore/src/EventSubscriber/DatastoreSubscriber.php index 82037ae412..45e687e1c8 100644 --- a/modules/datastore/src/EventSubscriber/DatastoreSubscriber.php +++ b/modules/datastore/src/EventSubscriber/DatastoreSubscriber.php @@ -2,12 +2,11 @@ namespace Drupal\datastore\EventSubscriber; -use Drupal\Core\Config\ConfigFactoryInterface; -use Drupal\Core\Logger\LoggerChannelFactory; - use Drupal\common\Events\Event; use Drupal\common\DataResource; use Drupal\common\Storage\JobStoreFactory; +use Drupal\Core\Config\ConfigFactoryInterface; +use Drupal\Core\Logger\LoggerChannelFactory; use Drupal\datastore\DatastoreService; use Drupal\datastore\Service\ResourceLocalizer; use Drupal\datastore\Service\ResourcePurger; @@ -15,8 +14,6 @@ use Drupal\metastore\LifeCycle\LifeCycle; use Drupal\metastore\MetastoreItemInterface; use Drupal\metastore\ResourceMapper; - -use Drupal\datastore\Plugin\QueueWorker\ImportJob; use Symfony\Component\DependencyInjection\ContainerInterface; use Symfony\Component\EventDispatcher\EventSubscriberInterface; @@ -75,6 +72,8 @@ public static function create(ContainerInterface $container) { * The dkan.datastore.service.resource_purger service. * @param \Drupal\common\Storage\JobStoreFactory $jobStoreFactory * The dkan.common.job_store service. + * @param \Drupal\datastore\Storage\ImportJobStoreFactory $importJobStoreFactory + * The dkan.datastore.import_job_store_factory service. */ public function __construct( ConfigFactoryInterface $config_factory, diff --git a/modules/datastore/src/Service/Import.php b/modules/datastore/src/Service/Import.php index 498e432e31..f4c34b1f1a 100644 --- a/modules/datastore/src/Service/Import.php +++ b/modules/datastore/src/Service/Import.php @@ -3,8 +3,8 @@ namespace Drupal\datastore\Service; use Drupal\common\DataResource; -use Drupal\common\Storage\JobStoreFactory; use Drupal\datastore\Storage\DatabaseTableFactory; +use Drupal\datastore\Storage\ImportJobStoreFactory; /** * Datastore import service. @@ -19,12 +19,12 @@ class Import extends ImportService { * * @param \Drupal\common\DataResource $resource * DKAN Resource. - * @param \Drupal\common\Storage\JobStoreFactory $importJobStoreFactory - * Jobstore factory. + * @param \Drupal\datastore\Storage\ImportJobStoreFactory $importJobStoreFactory + * Import jobstore factory. * @param \Drupal\datastore\Storage\DatabaseTableFactory $databaseTableFactory * Database Table factory. */ - public function __construct(DataResource $resource, JobStoreFactory $importJobStoreFactory, DatabaseTableFactory $databaseTableFactory) { + public function __construct(DataResource $resource, ImportJobStoreFactory $importJobStoreFactory, DatabaseTableFactory $databaseTableFactory) { parent::__construct($resource, $importJobStoreFactory, $databaseTableFactory); @trigger_error(__NAMESPACE__ . '\Import is deprecated. Use \Drupal\datastore\Service\ImportService instead.', E_USER_DEPRECATED); } From 1792eb4f93b3b9be88aeecd1455d6c56bd9e2654 Mon Sep 17 00:00:00 2001 From: Paul Mitchum Date: Tue, 26 Dec 2023 12:11:19 -0800 Subject: [PATCH 7/7] cs --- modules/datastore/src/DatastoreService.php | 2 ++ modules/datastore/src/Service/Factory/Import.php | 6 +++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/modules/datastore/src/DatastoreService.php b/modules/datastore/src/DatastoreService.php index a44a0eae71..77c07cf00e 100644 --- a/modules/datastore/src/DatastoreService.php +++ b/modules/datastore/src/DatastoreService.php @@ -87,6 +87,8 @@ public static function create(ContainerInterface $container) { * Queue factory service. * @param \Drupal\common\Storage\JobStoreFactory $jobStoreFactory * Jobstore factory service. + * @param \Drupal\datastore\Storage\ImportJobStoreFactory $importJobStoreFactory + * Import jobstore factory service. * @param \Drupal\datastore\Service\ResourceProcessor\DictionaryEnforcer $dictionaryEnforcer * Dictionary Enforcer object. */ diff --git a/modules/datastore/src/Service/Factory/Import.php b/modules/datastore/src/Service/Factory/Import.php index f61c4f7104..4f5ddc317e 100644 --- a/modules/datastore/src/Service/Factory/Import.php +++ b/modules/datastore/src/Service/Factory/Import.php @@ -3,7 +3,6 @@ namespace Drupal\datastore\Service\Factory; use Drupal\datastore\Storage\DatabaseTableFactory; -use Drupal\common\Storage\JobStoreFactory; use Drupal\datastore\Storage\ImportJobStoreFactory; /** @@ -16,6 +15,11 @@ class Import extends ImportServiceFactory { /** * Constructor. + * + * @param \Drupal\datastore\Storage\ImportJobStoreFactory $importJobStoreFactory + * Import job store factory service. + * @param \Drupal\datastore\Storage\DatabaseTableFactory $databaseTableFactory + * Database table factory. */ public function __construct(ImportJobStoreFactory $importJobStoreFactory, DatabaseTableFactory $databaseTableFactory) { parent::__construct($importJobStoreFactory, $databaseTableFactory);