pomm-project / ModelManager

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

Wrong FlexibleEntity status on modification #46

Closed ishenkoyv closed 9 years ago

ishenkoyv commented 9 years ago

According to FlexibleEntityInterface there are 3 statuses, but on existing in DB entity modification we get not existing status $entity->status() == 3. This is caused by StatefulEntityTrait::touch method as $this->status = 1 and 1 | 2 == 3.

To replicate

$proposal = $this->pomm()->getModel(ProposalModel::class)               
            ->findByPk(['proposal_id' => $this->params('proposal_id')]);

echo $proposal->status(); // 1
$proposal->setDeadline('10/10/2016');
$proposal->status(); // 3 BUT should be 2
chanmix51 commented 9 years ago

AFAIU, the entity is persisted and has been modified in memory so

$entity->getStatus() & FlexibleEntityInterface::STATUS_EXIST; // return true
$entity->getStatus() & FlexibleEntityInterface::STATUS_MODIFIED; // return true as well

So status is a linear combination of 1×1 + 1×2 = 3 (both modified and persisted).

Is there something I am missing ?

ishenkoyv commented 9 years ago

There is no such status as 3 and if I call get method I want to get current status without bitwise operations. Check your phpdoc for FlexibleEntityInterface::status(). Also StatefulEntityTrait::status() method should check for available statuses before assign new value.

chanmix51 commented 9 years ago

That's true, the doc block doesn't state it can also be a combination of 4 different possible states: 0 = none 1 = persisted 2 = modified 3 = persisted + modified

If a new state is needed (let's say TAINTED) it would be 4 and this will give 8 possible states for the FlexibleEntity. I am going to make the doc block more meaningful about that.

There used to be isModified() and isPersisted() methods in Pomm 1 but they are not present in Pomm2 since using bitwise operations with named constants makes the code self explained and more flexible.

True, the status($status) method does not validate the status. Since the status mechanism is extensible I think it would be a problem to limit the status system. Any ideas ?

ishenkoyv commented 9 years ago

https://github.com/pomm-project/ModelManager/blob/master/sources/lib/Model/FlexibleEntity/StatefulEntityTrait.php#L34 Is it ok that we can assign any value to state?

chanmix51 commented 9 years ago

I have updated my previous post on that topic. From my point of view, yes it is ok since developers can declare new netmask constants(4, 8, 16 etc.) so I feel like setting unknown value to status (let's say 255) will still be at least PERSISTED and MODIFIED with unknown other states so it doesn't break the actual status mechanism.

Any other opinions welcome.

chanmix51 commented 9 years ago

You mean it would be nice to cast the parameter as an int to compensate for PHP lack of type check ?

ishenkoyv commented 9 years ago

Yes, at least. It would be great to have code like this too

if (!in_array($status, FlexibleEntityInterface::getStatuses())) {
  throw new FoundationException(...);
}
chanmix51 commented 9 years ago

:+1: to cast the status as integer.

Testing the available status is not that simple because status is a bitmask of possible states. getStatuses() would have to return all possible combinations of available statuses (given they are registered). Furthermore, this method getStatuses() would collide with an entity field named statuses (likely to happen).