From f9144ecbae92070cdd8540c6f8a7744e1dd440d0 Mon Sep 17 00:00:00 2001 From: Matthias Steffens Date: Fri, 10 Dec 2021 16:48:40 +0100 Subject: [PATCH 01/25] #220 Converts the Collection & CollectionRole model classes to use Doctrine ORM (using the Doctrine "Tree" extension to implement nested-set behavior) --- composer.json | 7 +- library/Opus/Db2/CollectionRepository.php | 143 +++++++ library/Opus/Db2/CollectionRoleRepository.php | 66 +++ library/Opus/Db2/Database.php | 77 +++- library/Opus/Model2/Collection.php | 391 ++++++++++++++++++ library/Opus/Model2/CollectionRole.php | 145 +++++++ tests/Opus/CollectionTest.php | 15 +- tests/Opus/TestAsset/NestedSetValidator.php | 45 +- 8 files changed, 827 insertions(+), 62 deletions(-) create mode 100644 library/Opus/Db2/CollectionRepository.php create mode 100644 library/Opus/Db2/CollectionRoleRepository.php create mode 100644 library/Opus/Model2/Collection.php create mode 100644 library/Opus/Model2/CollectionRole.php diff --git a/composer.json b/composer.json index 3ec1e1ba..104737d8 100644 --- a/composer.json +++ b/composer.json @@ -18,8 +18,11 @@ "opus4-repo/opus4-common": "dev-master", "opus4-repo/opus4-doi": "4.7.*", "doctrine/dbal": "2.13.*", - "doctrine/orm": "2.10.*", - "doctrine/cache": "1.12.*" + "doctrine/orm": "^2", + "doctrine/cache": "1.12.*", + "doctrine/common": "^2.11", + "doctrine/persistence": "^1.3", + "gedmo/doctrine-extensions": "^2.4" }, "autoload": { "psr-4": { diff --git a/library/Opus/Db2/CollectionRepository.php b/library/Opus/Db2/CollectionRepository.php new file mode 100644 index 00000000..fe5185be --- /dev/null +++ b/library/Opus/Db2/CollectionRepository.php @@ -0,0 +1,143 @@ +findAll(); + } + + /** + * Returns all Collection nodes with the given role ID. Always returns an array, even if the + * result set has zero or one element. + * + * @param int $roleId The ID of the tree structure whose Collection nodes shall be returned. + * @param bool $sortResults (Optional) If true sort results by left ID. + * @return Collection[] + */ + public function fetchCollectionsByRoleId($roleId, $sortResults = false) + { + if (! isset($roleId)) { + throw new Exception("Parameter 'roleId' is required."); + } + + $queryBuilder = $this->getEntityManager()->createQueryBuilder(); + + $select = $queryBuilder->select('c') + ->from(Collection::class, 'c') + ->where('c.roleId = :roleId') + ->setParameter('roleId', $roleId); + + if ($sortResults === true) { + $select->orderBy('c.left', 'ASC'); + } + + $query = $select->getQuery(); + + // TODO double check that getResult() always returns an array + return $query->getResult(); + } + + /** + * Returns all Collection nodes with the given role ID & name. Always returns an array, even if + * the result set has zero or one element. + * + * @param int $roleId The ID of the tree structure whose Collection nodes shall be returned. + * @param string $name + * @return Collection[] + */ + public function fetchCollectionsByRoleName($roleId, $name) + { + if (! isset($roleId)) { + throw new Exception("Parameter 'roleId' is required."); + } + + if (! isset($name)) { + throw new Exception("Parameter 'name' is required."); + } + + $queryBuilder = $this->getEntityManager()->createQueryBuilder(); + + $query = $queryBuilder->select('c') + ->from(Collection::class, 'c') + ->where('c.roleId = :roleId') + ->andWhere('c.name = :name') + ->setParameter('roleId', $roleId) + ->setParameter('name', $name) + ->getQuery(); + + // TODO double check that getResult() always returns an array + return $query->getResult(); + } + + /** + * Returns all child nodes of the Collection node with given ID. + * + * @param int $parentId The ID of the node whose children shall be returned. + * @param bool $sortResults (Optional) If true sort results by left ID. + * @return Collection[] + */ + public function fetchChildrenByParentId($parentId, $sortResults = false) + { + if (! isset($parentId)) { + throw new Exception("Parameter 'parentId' is required."); + } + + $queryBuilder = $this->getEntityManager()->createQueryBuilder(); + + $select = $queryBuilder->select('c') + ->from(Collection::class, 'c') + ->where('c.parentId = :parentId') + ->setParameter('parentId', $parentId); + + if ($sortResults === true) { + $select->orderBy('c.left', 'ASC'); + } + + $query = $select->getQuery(); + + return $query->getResult(); + } +} diff --git a/library/Opus/Db2/CollectionRoleRepository.php b/library/Opus/Db2/CollectionRoleRepository.php new file mode 100644 index 00000000..fcb72872 --- /dev/null +++ b/library/Opus/Db2/CollectionRoleRepository.php @@ -0,0 +1,66 @@ +findAll(); + } + + /** + * Retrieves an existing CollectionRole instance by name. Returns + * null if name is null *or* if nothing was found. + * + * @param string|null $name + * @return CollectionRole|null + */ + public function fetchByName($name = null) + { + if ($name === null) { + return null; + } + + return $this->findOneBy(['name' => $name]); + } +} diff --git a/library/Opus/Db2/Database.php b/library/Opus/Db2/Database.php index 757fed30..20147cca 100644 --- a/library/Opus/Db2/Database.php +++ b/library/Opus/Db2/Database.php @@ -31,12 +31,19 @@ namespace Opus\Db2; +use Doctrine\Common\Annotations\AnnotationReader; +use Doctrine\Common\Annotations\AnnotationRegistry; +use Doctrine\Common\Annotations\CachedReader; +use Doctrine\Common\Persistence\Mapping\Driver\MappingDriverChain; use Doctrine\DBAL\Connection; use Doctrine\DBAL\DriverManager; use Doctrine\DBAL\Exception; use Doctrine\ORM\EntityManager; +use Doctrine\ORM\Mapping\Driver\AnnotationDriver; use Doctrine\ORM\ORMException; use Doctrine\ORM\Tools\Setup; +use Gedmo\DoctrineExtensions; +use Gedmo\Tree\TreeListener; use Opus\Config; use Opus\Database as OpusDatabase; @@ -123,31 +130,71 @@ public static function getConnection() * @return EntityManager * @throws ORMException * - * TODO more specific than __DIR__? Why __DIR__? - * TODO evaluate, explain options + * TODO reevaluate setup for Doctrine & DoctrineExtensions + * TODO more specific than __DIR__? + * TODO use PsrCachedReader instead of CachedReader */ public static function getEntityManager() { + if (self::$entityManager !== null) { + return self::$entityManager; + } + $config = Config::get(); + $paths = [__DIR__]; // one or multiple paths where mapping classes can be found $isDevMode = filter_var($config->doctrine->devMode, FILTER_VALIDATE_BOOLEAN); - $proxyDir = null; // TODO What could go here? - $cache = null; // TODO What could go here? - $useSimpleAnnotationReader = false; - - $config = Setup::createAnnotationMetadataConfiguration( - [__DIR__], - $isDevMode, - $proxyDir, - $cache, - $useSimpleAnnotationReader + $proxyDir = null; // the directory where Doctrine generates any necessary proxy class files + $cache = null; // used to cache metadata, query & result, pass null to auto-generate caches + $useSimpleAnnotationReader = false; // if true, `@Entity` will work, otherwise `@ORM\Entity` will be supported + + // NOTE: Instead of just using Setup::createAnnotationMetadataConfiguration() as below to create the $config + // registration of the Doctrine "Tree" extension (Gedmo\DoctrineExtensions) requires a more manual setup. + // The Doctrine "Tree" extension implements nested-set behavior for Doctrine. + +// $config = Setup::createAnnotationMetadataConfiguration( +// $paths, +// $isDevMode, +// $proxyDir, +// $cache, +// $useSimpleAnnotationReader +// ); + + $config = Setup::createConfiguration($isDevMode, $proxyDir, $cache); + + // Ensure standard Doctrine annotations are registered + AnnotationRegistry::registerFile( + __DIR__ . '/../../../vendor/doctrine/orm/lib/Doctrine/ORM/Mapping/Driver/DoctrineAnnotations.php' ); + // Use the (metadata) cache that was auto-generated when creating the configuration + $metadataCache = $config->getMetadataCacheImpl(); + + // Build the annotation reader for the application + $cachedReader = new CachedReader(new AnnotationReader(), $metadataCache); + + // Create the mapping driver chain that will be used to read metadata from our various sources + $mappingDriver = new MappingDriverChain(); + + // Load the superclass metadata mapping for the Doctrine extensions into the driver chain. + // Internally, this will also register the Doctrine Extensions annotations + DoctrineExtensions::registerAbstractMappingIntoDriverChainORM($mappingDriver, $cachedReader); + + $annotationDriver = new AnnotationDriver($cachedReader, $paths); + + // Register the application entities to our driver chain + $mappingDriver->addDriver($annotationDriver, 'Opus'); + + $config->setMetadataDriverImpl($mappingDriver); + $conn = self::getConnection(); - if (self::$entityManager === null) { - self::$entityManager = EntityManager::create($conn, $config); - } + // Add a listener for the Doctrine "Tree" extension + $treeListener = new TreeListener(); + $eventManager = $conn->getEventManager(); + $eventManager->addEventSubscriber($treeListener); + + self::$entityManager = EntityManager::create($conn, $config, $eventManager); return self::$entityManager; } diff --git a/library/Opus/Model2/Collection.php b/library/Opus/Model2/Collection.php new file mode 100644 index 00000000..e8db8e1a --- /dev/null +++ b/library/Opus/Model2/Collection.php @@ -0,0 +1,391 @@ +id; + } + + /** + * @return int + */ + public function getLeft() + { + return $this->left; + } + + /** + * @return int + */ + public function getRight() + { + return $this->right; + } + + /** + * @return int + */ + public function getParentId() + { + return $this->parentId; + } + + /** + * @return self|null + */ + public function getParent() + { + return $this->parent; + } + + /** + * @param self|null $parent + * @return $this + */ + public function setParent($parent) + { + $this->parent = $parent; + $this->parentId = $parent->getId(); + + return $this; + } + + /** + * @return ORMCollection|self[] + */ + public function getChildren() + { + return $this->children; + } + + /** + * @return int + */ + public function getRoleId() + { + return $this->roleId; + } + + /** + * @return CollectionRole + */ + public function getRole() + { + return $this->role; + } + + /** + * @param CollectionRole $role + * @return $this + */ + public function setRole($role) + { + $this->role = $role; + $this->roleId = $role->getId(); + + return $this; + } + + /** + * @return string + */ + public function getNumber() + { + return $this->number; + } + + /** + * @param string $number + * @return $this + */ + public function setNumber($number) + { + $this->number = $number; + + return $this; + } + + /** + * @return string + */ + public function getName() + { + return $this->name; + } + + /** + * @param string $name + * @return $this + */ + public function setName($name) + { + $this->name = $name; + + return $this; + } + + /** + * @return string + */ + public function getOaiSubset() + { + return $this->oaiSubset; + } + + /** + * @param string $oaiSubset + * @return $this + */ + public function setOaiSubset($oaiSubset) + { + $this->oaiSubset = $oaiSubset; + + return $this; + } + + /** + * @return int + */ + public function getVisible() + { + return $this->visible; + } + + /** + * @param int $visible + * @return $this + */ + public function setVisible($visible) + { + $this->visible = $visible; + + return $this; + } + + /** + * @return int + */ + public function getVisiblePublish() + { + return $this->visiblePublish; + } + + /** + * @param int $visiblePublish + * @return $this + */ + public function setVisiblePublish($visiblePublish) + { + $this->visiblePublish = $visiblePublish; + + return $this; + } + + /** + * Retrieve all Collection instances from the database. + * + * @return self[] + */ + public static function getAll() + { + return self::getRepository()->getAll(); + } + + /** + * Returns all Collection nodes with the given role ID. Always returns an array, even if the + * result set has zero or one element. + * + * @param int $roleId The ID of the tree structure whose Collection nodes shall be returned. + * @param bool $sortResults (Optional) If true sort results by left ID. + * @return self[] + */ + public static function fetchCollectionsByRoleId($roleId, $sortResults = false) + { + return self::getRepository()->fetchCollectionsByRoleId($roleId, $sortResults); + } + + /** + * Returns all Collection nodes with the given role ID & name. Always returns an array, even if + * the result set has zero or one element. + * + * @param int $roleId The ID of the tree structure whose Collection nodes shall be returned. + * @param string $name + * @return self[] + */ + public static function fetchCollectionsByRoleName($roleId, $name) + { + return self::getRepository()->fetchCollectionsByRoleName($roleId, $name); + } + + /** + * Returns all child nodes of the Collection node with given ID. + * + * @param int $parentId The ID of the node whose children shall be returned. + * @param bool $sortResults (Optional) If true sort results by left ID. + * @return self[] + */ + public static function fetchChildrenByParentId($parentId, $sortResults = false) + { + return self::getRepository()->fetchChildrenByParentId($parentId, $sortResults); + } + + /** + * Returns the relevant properties of the class. + * + * @return array + */ + protected static function describe() + { + return ['Number', 'Name', 'OaiSubset']; + } +} diff --git a/library/Opus/Model2/CollectionRole.php b/library/Opus/Model2/CollectionRole.php new file mode 100644 index 00000000..1892cb0f --- /dev/null +++ b/library/Opus/Model2/CollectionRole.php @@ -0,0 +1,145 @@ +id; + } + + /** + * @return string + */ + public function getName() + { + return $this->name; + } + + /** + * @param string $name + * @return $this + */ + public function setName($name) + { + $this->name = $name; + + return $this; + } + + /** + * @return Collection + */ + public function getRootCollection() + { + return $this->rootCollection; + } + + /** + * @param Collection $rootCollection + * @return $this + */ + public function setRootCollection($rootCollection) + { + $this->rootCollection = $rootCollection; + + return $this; + } + + /** + * Retrieve all CollectionRole instances from the database. + * + * @return self[] + */ + public static function getAll() + { + return self::getRepository()->getAll(); + } + + /** + * Retrieves an existing CollectionRole instance by name. Returns + * null if name is null *or* if nothing was found. + * + * @param string|null $name + * @return self|null + */ + public static function fetchByName($name = null) + { + return self::getRepository()->fetchByName($name); + } + + /** + * Returns the relevant properties of the class. + * + * @return array + */ + protected static function describe() + { + return ['Name']; + } +} diff --git a/tests/Opus/CollectionTest.php b/tests/Opus/CollectionTest.php index 62c3d736..57b09937 100644 --- a/tests/Opus/CollectionTest.php +++ b/tests/Opus/CollectionTest.php @@ -40,13 +40,12 @@ use Exception; use InvalidArgumentException; -use Opus\Collection; use Opus\CollectionRole; use Opus\Config; -use Opus\Db\Collections; use Opus\Document; use Opus\Model\NotFoundException; use Opus\Model\Xml\Cache; +use Opus\Model2\Collection; use OpusTest\Model\Plugin\AbstractPluginMock; use OpusTest\TestAsset\NestedSetValidator; use OpusTest\TestAsset\TestCase; @@ -1233,18 +1232,14 @@ public function testSortChildrenBySpecifiedOrderBadId() */ protected function validateNestedSet() { - $table = new Collections(); - - $select = $table->select()->where('role_id = ?', 1)->order('left_id ASC'); - - $rows = $table->fetchAll($select); + $rows = Collection::fetchCollectionsByRoleId(1, true); $this->assertEquals(14, count($rows)); - $this->assertEquals(1, $rows[0]->left_id); - $this->assertEquals(28, $rows[0]->right_id); + $this->assertEquals(1, $rows[0]->getLeft()); + $this->assertEquals(28, $rows[0]->getRight()); - $validator = new NestedSetValidator($table); + $validator = new NestedSetValidator(); $this->assertTrue($validator->validate(1)); } diff --git a/tests/Opus/TestAsset/NestedSetValidator.php b/tests/Opus/TestAsset/NestedSetValidator.php index 9221f0cb..6d9532f3 100644 --- a/tests/Opus/TestAsset/NestedSetValidator.php +++ b/tests/Opus/TestAsset/NestedSetValidator.php @@ -35,9 +35,8 @@ namespace OpusTest\TestAsset; -use Opus\Db\NestedSet; use Opus\LoggingTrait; -use Opus\Model\ModelException; +use Opus\Model2\Collection; /** * Validates the structure of a NestedSet in the database. @@ -46,13 +45,6 @@ class NestedSetValidator { use LoggingTrait; - /** - * Database table adapter. - * - * @var NestedSet - */ - private $table; - /** * Position counter in NestedSet. * @@ -60,19 +52,6 @@ class NestedSetValidator */ private $counter; - /** - * @param NestedSet $table - * @throws ModelException - */ - public function __construct($table) - { - if ($table !== null && $table instanceof NestedSet) { - $this->table = $table; - } else { - throw new ModelException('object must be instance of Opus\Db\NestedSet'); - } - } - /** * Check structure of nested set. * @@ -81,9 +60,9 @@ public function __construct($table) */ public function validate($rootId) { - $select = $this->table->select()->where('id = ?', $rootId); - $node = $this->table->fetchRow($select); - $this->counter = (int) $node['left_id']; + /** @var Collection $node */ + $node = Collection::get($rootId); + $this->counter = $node->getLeft(); return $this->validateNode($rootId); // root node } @@ -97,10 +76,10 @@ public function validateNode($nodeId) { $logger = $this->getLogger(); - $select = $this->table->select()->where('id = ?', $nodeId); - $node = $this->table->fetchRow($select); - $leftId = $node['left_id']; - $rightId = $node['right_id']; + /** @var Collection $node */ + $node = Collection::get($nodeId); + $leftId = $node->getLeft(); + $rightId = $node->getRight(); $logger->err("{$this->counter}, $nodeId: $leftId, $rightId"); @@ -121,13 +100,9 @@ public function validateNode($nodeId) // node if ($distance & 1) { // odd; valid - $selectChildren = $this->table->select()->where( - 'parent_id = ?', - $nodeId - )->order('left_id ASC'); - $children = $this->table->fetchAll($selectChildren); + $children = Collection::fetchChildrenByParentId($nodeId, true); foreach ($children as $child) { - if ($this->validateNode($child['id']) === false) { + if ($this->validateNode($child->getId()) === false) { return false; } } From c0ad048a01bb2f002dcf31d3d5ec5e2da7eaaab4 Mon Sep 17 00:00:00 2001 From: j3nsch Date: Wed, 5 Jan 2022 09:28:55 +0100 Subject: [PATCH 02/25] #220 Use PHP 7.2 for tests --- .github/workflows/php.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/php.yml b/.github/workflows/php.yml index 940b0737..6a98635e 100644 --- a/.github/workflows/php.yml +++ b/.github/workflows/php.yml @@ -17,10 +17,10 @@ jobs: steps: - uses: actions/checkout@v2 - - name: Setup PHP 7.1 + - name: Setup PHP 7.2 uses: shivammathur/setup-php@v2 with: - php-version: '7.1' + php-version: '7.2' - name: Update Ubuntu packages run: sudo apt-get update From eab17e4797e2b552ebb25435f09e140ebcc3a40b Mon Sep 17 00:00:00 2001 From: j3nsch Date: Wed, 5 Jan 2022 10:50:51 +0100 Subject: [PATCH 03/25] #220 Limit tests to CollectionTest.php --- .github/workflows/php.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/php.yml b/.github/workflows/php.yml index 6a98635e..49599394 100644 --- a/.github/workflows/php.yml +++ b/.github/workflows/php.yml @@ -38,7 +38,7 @@ jobs: run: ant prepare-workspace prepare-config create-database lint -DdbUserPassword=root -DdbAdminPassword=root - name: Test - run: php composer.phar test + run: php composer.phar test tests/Opus/CollectionTest.php - name: Coding-Style run: php composer.phar cs-check From b307f7fd3c0ce8c4ed84718044bd1ea0dd86b4bc Mon Sep 17 00:00:00 2001 From: Matthias Steffens Date: Thu, 10 Feb 2022 10:31:43 +0100 Subject: [PATCH 04/25] #220 For the Vagrantfile, use PHP 7.2 as required by gedmo/doctrine-extensions 3.0 --- Vagrantfile | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/Vagrantfile b/Vagrantfile index 868cbb29..a8f1fc4b 100644 --- a/Vagrantfile +++ b/Vagrantfile @@ -2,10 +2,10 @@ # vi: set ft=ruby : $software = <