pomm-project / ModelManager

Model Manager for the Pomm database framework.
MIT License
66 stars 27 forks source link

Convenient methods in flexible entities #5

Closed tlode closed 9 years ago

tlode commented 9 years ago

I've tried to implement a convenient method hasChildNodes in a FlexibleEntity sub class. This method determines its boolean return value from other attributes. There is no field child_nodes in the query or method getChildNodes() accordingly.

When I call extract() on this entity and getChildNodes() returns true, a ModelException is thrown:

No such key 'child_nodes'. in ..../vendor/pomm-project/model-manager/sources/lib/Model/FlexibleEntity.php on line 71

FlexibleEntity::getIterator() tries to resolve the custom attribute child_nodes via the get* method, which fails because there is no such attribute or method.

I've looked into the code and found that this is part of the overloading feature which is mentioned in the docs and in your comment, stating that it is possible to implement overload methods get* if the according has* method is also implemented.

But having a has* method without a get* method won't work. I am not sure if this is a bug or could somehow circumvented. I would really like being able to use both possibilities.

chanmix51 commented 9 years ago

Can you please post the complete stack trace of the Exception with the content of your entity code ? Or maybe there is a repo where we can browse the code ?

tlode commented 9 years ago

Sorry for leaving this out:

class NestedNodes extends FlexibleEntity
{
    public function hasChildNodes()
    {
        return true;
    }
}
// nestedNodesModel is the bare generated model sub class
$node = $nestedNodesModel->createEntity(['lft' => 1,
                                          'rgt' => 2,
                                          'level' => 0
                                         ]);

print_r($node->extract());
PommProject\ModelManager\Exception\ModelException: No such key 'child_nodes'. in /Users/pronoia/www/pomm-test-simple/vendor/pomm-project/model-manager/sources/lib/Model/FlexibleEntity.php on line 71

Call Stack:
    0.0003     228096   1. {main}() /Users/pronoia/www/pomm-test-simple/index.php:0
    0.0638    1452984   2. PommProject\ModelManager\Model\FlexibleEntity->extract() /Users/pronoia/www/pomm-test-simple/index.php:13
    0.0638    1453936   3. PommProject\ModelManager\Model\FlexibleEntity->getIterator() /Users/pronoia/www/pomm-test-simple/vendor/pomm-project/model-manager/sources/lib/Model/FlexibleEntity.php:266
    0.0641    1455328   4. call_user_func:{/Users/pronoia/www/pomm-test-simple/vendor/pomm-project/model-manager/sources/lib/Model/FlexibleEntity.php:411}() /Users/pronoia/www/pomm-test-simple/vendor/pomm-project/model-manager/sources/lib/Model/FlexibleEntity.php:411
    0.0641    1455496   5. MyDb\PublicSchema\NestedNodes->getChildNodes() /Users/pronoia/www/pomm-test-simple/vendor/pomm-project/model-manager/sources/lib/Model/FlexibleEntity.php:411
    0.0641    1455856   6. PommProject\ModelManager\Model\FlexibleEntity->__call() /Users/pronoia/www/pomm-test-simple/vendor/pomm-project/model-manager/sources/lib/Model/FlexibleEntity.php:411
    0.0641    1456488   7. PommProject\ModelManager\Model\FlexibleEntity->get() /Users/pronoia/www/pomm-test-simple/vendor/pomm-project/model-manager/sources/lib/Model/FlexibleEntity.php:184
tlode commented 9 years ago

You can browse or clone my simple test app from https://github.com/tlode/pomm-test-simple

chanmix51 commented 9 years ago

Ok I understand better. The documentation might be unclear on how extract() works.

If a method hasX() exists and returns true, then the method getX() is called to get the computed value. If this method does not exist, Pomm does not know how to return the extra attribute and throws an exception.

I do not really understand what you are trying to achieve but if you are trying to store hierarchical values, you probably do not want to use the Nested Set Pattern but a foreign key parent_id with a recursive query (like the one in Pomm1 tutorial) might do the job better.

If you want help about these features, I would suggest using the mailing list.

If you are ok I can close this issue.

tlode commented 9 years ago

Ok, thanks. I think I understand the hasX() / getX() feature. But I am not sure, if I understand how you would then get a computed boolean state info through an entity method which starts with has (whereas starting with is wouldn't be a problem)?

Suppose I do want to know a certain state, like hasNoErrors() on Task returning count($this->errors) or hasChildNodes() on NestedNode returning ($this->rgt - $this->lft) > 1). Shouldn't that be possible with flexible entities as well?

Why not enabling the hasX() / getX() feature only, if both methods are present. Something like this would solve my problem and suites all the tests in model-manager:

diff --git a/sources/lib/Model/FlexibleEntity.php b/sources/lib/Model/FlexibleEntity.php
index 6bae53e..7255d93 100644
--- a/sources/lib/Model/FlexibleEntity.php
+++ b/sources/lib/Model/FlexibleEntity.php
@@ -432,7 +432,9 @@ abstract class FlexibleEntity implements \ArrayAccess, \IteratorAggregate

         foreach (get_class_methods($entity) as $method) {
             if (preg_match('/^has([A-Z].*)$/', $method, $matchs)) {
-                static::$has_methods[] = $matchs[1];
+                if (method_exists($entity, 'get'.$matchs[1])) {
+                    static::$has_methods[] = $matchs[1];
+                }
             }
         }
     }
chanmix51 commented 9 years ago

Yes but this approach breaks the case where you overload a "native" hasX() method that does not have a builtin getX() but the call still caught by __call().

Even though the patch you do propose is simple, I am a bit reluctant to get the FlexibleEntity more complex than it is (and according to scrutinizer it is by far the most complex class of this package).

I would be more favorable to an approach where getX(), setX(), hasX and addX() are reserved for the flexible entity attribute management so people knowing Pomm and reading someone else's code would not be confused by a hasX method not declaring a getX attribute.

I would be happy to read more opinions about this, any ideas ?

chanmix51 commented 9 years ago

Can I close this issue now ?

tlode commented 9 years ago

Yes, thanks.