From 5d43137f3ccc6848599a3f8f52f6a141563af96e Mon Sep 17 00:00:00 2001 From: Dennis Eichhorn Date: Sun, 12 Jan 2020 17:57:18 +0100 Subject: [PATCH] fix todo comments --- DataStorage/Database/DataMapperAbstract.php | 119 +++++++++++--------- Stdlib/Graph/BinaryTree.php | 29 +++-- Stdlib/Graph/Node.php | 14 +++ Utils/TaskSchedule/Schedule.php | 5 +- 4 files changed, 95 insertions(+), 72 deletions(-) diff --git a/DataStorage/Database/DataMapperAbstract.php b/DataStorage/Database/DataMapperAbstract.php index 451592e8b..a1f9496c0 100644 --- a/DataStorage/Database/DataMapperAbstract.php +++ b/DataStorage/Database/DataMapperAbstract.php @@ -410,8 +410,6 @@ class DataMapperAbstract implements DataMapperInterface // clear parent and objects if (static::class === self::$parentMapper) { - //self::$initObjects = []; // todo: now all objects are cached for the whole request - //self::$initArrays = []; // todo: now all objects are cached for the whole request self::$parentMapper = null; } } @@ -461,7 +459,15 @@ class DataMapperAbstract implements DataMapperInterface $refClass = new \ReflectionClass($obj); - // todo: remove force and instead check if object is autoincrement. if not autoincrement then create even if id is set!!! + /** + * @todo Orange-Management/phpOMS#234 + * Add forced IDs to the mappers + * Currently it is required to manually create a model with forced ids. + * This should be defined in the mappers in the ID column definition. + * ``` + * 'force_id' => true, + * ``` + */ if (!empty($id = self::getObjectId($obj, $refClass)) && !$force) { $objId = $id; } else { @@ -561,12 +567,8 @@ class DataMapperAbstract implements DataMapperInterface $query->insert($column['name'])->value($value); } elseif ($column['name'] !== static::$primaryField || !empty($property->getValue($obj))) { $tValue = $property->getValue($obj); - if (\stripos($column['internal'], '/') !== false) { - $path = \explode('/', $column['internal']); - - \array_shift($path); - $path = \implode('/', $path); - $tValue = ArrayUtils::getArray($path, $tValue, '/'); + if (\stripos($column['internal'], '/') === 0) { + $tValue = ArrayUtils::getArray($column['internal'], $tValue, '/'); } $value = self::parseValue($column['type'], $tValue); @@ -633,14 +635,11 @@ class DataMapperAbstract implements DataMapperInterface } $path = $column['internal']; - if (\stripos($column['internal'], '/') !== false) { - $path = \explode('/', $column['internal']); - - \array_shift($path); // todo: why am I doing this? - $path = \implode('/', $path); + if (\stripos($column['internal'], '/') === 0) { + $path = \ltrim($column['internal'], '/'); } - $property = ArrayUtils::getArray($path, $obj, '/'); + $property = ArrayUtils::getArray($column['internal'], $obj, '/'); if (isset(static::$ownsOne[$path])) { $id = self::createOwnsOneArray($column['internal'], $property); @@ -1194,7 +1193,6 @@ class DataMapperAbstract implements DataMapperInterface $objsIds[$propertyName] = []; foreach ($values as $key => &$value) { - // todo: carefull what if a value is an object or another array? if (!\is_array($value)) { // Is scalar => already in database $objsIds[$propertyName][$key] = $value; @@ -1309,8 +1307,6 @@ class DataMapperAbstract implements DataMapperInterface /** @var string $mapper */ $mapper = static::$ownsOne[$propertyName]['mapper']; - // todo: delete owned one object is not recommended since it can be owned by by something else? or does owns one mean that nothing else can have a relation to this one? - return $mapper::update($obj, $relations, $depth); } @@ -1429,23 +1425,30 @@ class DataMapperAbstract implements DataMapperInterface $id = self::updateOwnsOne($propertyName, $property->getValue($obj), $relations, $depth); $value = self::parseValue($column['type'], $id); - // todo: should not be done if the id didn't change. but for now don't know if id changed + /** + * @todo Orange-Management/phpOMS#232 + * If a model gets updated all it's relations are also updated. + * This should be prevented if the relations didn't change. + * No solution yet. + */ $query->set([static::$table . '.' . $column['name'] => $value]); } elseif (isset(static::$belongsTo[$propertyName])) { $id = self::updateBelongsTo($propertyName, $property->getValue($obj), $relations, $depth); $value = self::parseValue($column['type'], $id); - // todo: should not be done if the id didn't change. but for now don't know if id changed + /** + * @todo Orange-Management/phpOMS#232 + * If a model gets updated all it's relations are also updated. + * This should be prevented if the relations didn't change. + * No solution yet. + */ $query->set([static::$table . '.' . $column['name'] => $value]); } elseif ($column['name'] !== static::$primaryField) { $tValue = $property->getValue($obj); - if (\stripos($column['internal'], '/') !== false) { - $path = \explode('/', $column['internal']); - - \array_shift($path); - $path = \implode('/', $path); - $tValue = ArrayUtils::getArray($path, $tValue, '/'); + if (\stripos($column['internal'], '/') === 0) { + $tValue = ArrayUtils::getArray($column['internal'], $tValue, '/'); } + $value = self::parseValue($column['type'], $tValue); $query->set([static::$table . '.' . $column['name'] => $value]); @@ -1492,13 +1495,10 @@ class DataMapperAbstract implements DataMapperInterface if ($column['name'] !== $conditional['dst']) { $tValue = $property->getValue($obj); - if (\stripos($column['internal'], '/') !== false) { - $path = \explode('/', $column['internal']); - - \array_shift($path); - $path = \implode('/', $path); - $tValue = ArrayUtils::getArray($path, $tValue, '/'); + if (\stripos($column['internal'], '/') === 0) { + $tValue = ArrayUtils::getArray($column['internal'], $tValue, '/'); } + $value = self::parseValue($column['type'], $tValue); $query->set([$table . '.' . $column['name'] => $value]); @@ -1538,26 +1538,33 @@ class DataMapperAbstract implements DataMapperInterface } $path = $column['internal']; - if (\stripos($column['internal'], '/') !== false) { - $path = \explode('/', $column['internal']); - - \array_shift($path); // todo: why am I doing this? - $path = \implode('/', $path); + if (\stripos($column['internal'], '/') === 0) { + $path = \ltrim($column['internal'], '/'); } - $property = ArrayUtils::getArray($path, $obj, '/'); + $property = ArrayUtils::getArray($column['internal'], $obj, '/'); if (isset(static::$ownsOne[$path])) { $id = self::updateOwnsOneArray($column['internal'], $property, $relations, $depth); $value = self::parseValue($column['type'], $id); - // todo: should not be done if the id didn't change. but for now don't know if id changed + /** + * @todo Orange-Management/phpOMS#232 + * If a model gets updated all it's relations are also updated. + * This should be prevented if the relations didn't change. + * No solution yet. + */ $query->set([static::$table . '.' . $column['name'] => $value]); } elseif (isset(static::$belongsTo[$path])) { $id = self::updateBelongsToArray($column['internal'], $property, $relations, $depth); $value = self::parseValue($column['type'], $id); - // todo: should not be done if the id didn't change. but for now don't know if id changed + /** + * @todo Orange-Management/phpOMS#232 + * If a model gets updated all it's relations are also updated. + * This should be prevented if the relations didn't change. + * No solution yet. + */ $query->set([static::$table . '.' . $column['name'] => $value]); } elseif ($column['name'] !== static::$primaryField) { $value = self::parseValue($column['type'], $property); @@ -1590,15 +1597,7 @@ class DataMapperAbstract implements DataMapperInterface ->where($table . '.' . $conditional['dst'], '=', $objId); foreach ($conditional['columns'] as $key => $column) { - $path = $column['internal']; - if (\stripos($column['internal'], '/') !== false) { - $path = \explode('/', $column['internal']); - - \array_shift($path); // todo: why am I doing this? - $path = \implode('/', $path); - } - - $property = ArrayUtils::getArray($path, $obj, '/'); + $property = ArrayUtils::getArray($column['internal'], $obj, '/'); if ($column['name'] !== $conditional['dst']) { $value = self::parseValue($column['type'], $property); @@ -1762,8 +1761,11 @@ class DataMapperAbstract implements DataMapperInterface continue; } - // todo: could be a problem, relation needs to be removed first?! - + /** + * @todo Orange-Management/phpOMS#233 + * On delete the relations and relation tables need to be deleted first + * The exception is of course the belongsTo relation. + */ } self::deleteRelationTable($propertyName, $objsIds, $objId); @@ -1880,9 +1882,11 @@ class DataMapperAbstract implements DataMapperInterface $property->setAccessible(true); } - // todo: the order of deletion could be a problem. maybe looping through ownsOne and belongsTo first is better. - // todo: support other relation types as well (belongsto, ownsone) = for better control - + /** + * @todo Orange-Management/phpOMS#233 + * On delete the relations and relation tables need to be deleted first + * The exception is of course the belongsTo relation. + */ foreach (static::$columns as $key => $column) { if ($relations === RelationType::ALL && isset(static::$ownsOne[$propertyName]) && $column['internal'] === $propertyName) { self::deleteOwnsOne($propertyName, $property->getValue($obj)); @@ -2015,8 +2019,11 @@ class DataMapperAbstract implements DataMapperInterface } if (!isset($obj)) { - // todo: implement solution for classes with constructor arguments - // maybe implement a factory pattern for every datamapper model + /** + * @todo Orange-Management/phpOMS#67 + * Since some models require special initialization a model factory should be implemented. + * This could be a simple initialize() function in the mapper where the default initialize() is the current defined empty initialization in the DataMapperAbstract. + */ $obj = new $class(); } diff --git a/Stdlib/Graph/BinaryTree.php b/Stdlib/Graph/BinaryTree.php index de5f06122..e615644d8 100644 --- a/Stdlib/Graph/BinaryTree.php +++ b/Stdlib/Graph/BinaryTree.php @@ -49,11 +49,11 @@ class BinaryTree extends Tree * * @param Node $base Tree node * - * @return Node Left node + * @return null|Node Left node * * @since 1.0.0 */ - public function getLeft(Node $base) + public function getLeft(Node $base) : ?Node { $neighbors = $base->getNeighbors($base); @@ -66,11 +66,11 @@ class BinaryTree extends Tree * * @param Node $base Tree node * - * @return Node Right node + * @return null|Node Right node * * @since 1.0.0 */ - public function getRight(Node $base) + public function getRight(Node $base) : ?Node { $neighbors = $this->getNeighbors($base); @@ -198,8 +198,8 @@ class BinaryTree extends Tree /** * Check if tree is symmetric. * - * @param Node $node1 Tree node1 - * @param Node $node2 Tree node2 (optional, can be different tree) + * @param null|Node $node1 Tree node1 + * @param null|Node $node2 Tree node2 (optional, can be different tree) * * @return bool True if tree is symmetric, false if tree is not symmetric * @@ -207,21 +207,20 @@ class BinaryTree extends Tree */ public function isSymmetric(Node $node1 = null, Node $node2 = null) : bool { - if ($node1 === null && $node2 === null) { + if (($node1 === null && $node2 === null) + || $node1->isEqual($node2) + ) { return true; + } elseif ($node1 === null || $node2 === null) { + return false; } $left1 = $this->getLeft($node1); $right1 = $this->getRight($node1); - $left2 = $node2 !== null ? $this->getLeft($node1) : $this->getLeft($node2); - $right2 = $node2 !== null ? $this->getRight($node1) : $this->getRight($node2); + $left2 = $node2 !== null ? $this->getLeft($node1) : null; + $right2 = $node2 !== null ? $this->getRight($node1) : null; - // todo: compare values? true symmetry requires the values to be the same - if ($node1 !== null && $node2 !== null) { - return $this->isSymmetric($left1, $right2) && $this->isSymmetric($right1, $left2); - } - - return false; + return $this->isSymmetric($left1, $right2) && $this->isSymmetric($right1, $left2); } } diff --git a/Stdlib/Graph/Node.php b/Stdlib/Graph/Node.php index fe962dcc6..91c18f667 100644 --- a/Stdlib/Graph/Node.php +++ b/Stdlib/Graph/Node.php @@ -67,4 +67,18 @@ class Node { $this->data = $data; } + + /** + * Compare with other node. + * + * @param Node $node Node + * + * @return boll + * + * @since 1.0.0 + */ + public function isEqual(Node $node) : bool + { + return true; + } } diff --git a/Utils/TaskSchedule/Schedule.php b/Utils/TaskSchedule/Schedule.php index 0a7adb263..14cdf6a85 100644 --- a/Utils/TaskSchedule/Schedule.php +++ b/Utils/TaskSchedule/Schedule.php @@ -39,7 +39,10 @@ class Schedule extends TaskAbstract */ public static function createWith(array $jobData) : TaskAbstract { - // todo: fix interval this is just a dummy|!!! + /** + * @todo Orange-Management/phpOMS#231 + * Use the interval for generating a schedule + */ $job = new self($jobData[1], $jobData[8], 'asdf'); $job->setStatus($jobData[3]);