StatefulEntityTrait::status() method is confusing

[pounard]

It took me a while to realize, but reading this:

    public function status($status = null)
    {
        if ($status !== null) {
            $this->status = (int) $status;

            return $this;
        }

        return $this->status;
    }

I could see that when you set the status, you return the entity, but if you don't, you return the status, return type is varying and it's confusing.

How about a setStatus() and a getStatus() method instead ?

[sanpii]

👍

A method that make two differents thinks is not a good idea.

[pounard]

Actually there a few others, such as fields()

[chanmix51]

Yes, this is because getX, setX, addX etc. methods are protected for users use. We must not have a public inner method to be called like those. Since PSR says methods must not start with underscore, here is a work around but better solutions are welcome 👐

[chanmix51]

In other words, if a user has an attribute named status he will use methods getStatus(), setStatus() etc. We must not interfere with that API.

[tlode]

Why not make explicit setters? for example flagPersisted() for state EXIST, flagModified() for state MODIFIED and flagNew() for state NONE.

[chanmix51]

Afaicr there has been a similar discussion a year ago, I will try to sum it up here.

There are two kinds of independent states today : persisted, changed. This makes 4 possible combinations. If a third state is added (easily feasible today in a custom project) like 'TAINTED' by example, it will make 8 possible states hence it would mean the class to have 8 state methods. If another state is added it will be 16 and so on.

The internal state of an entity is a mask of states.

[tlode]

But they could work like StatefulEntityTrait::touch()

$this->status |= FlexibleEntityInterface::STATUS_MODIFIED;

Then, one can check a specific state with a method like flaggedModified()

public function flaggedModified()
{
    return (bool)($this->status & FlexibleEntityInterface::STATUS_MODIFIED)
}

What I was trying to say, is that choosing a different name for setters and getters might solve the problem with the SingleMethodDifferentReturnTypes problem

[pounard]

I do understand the problem with conflicting with model field names, and that's a problem will potentially all words in the english language :)

May be just specializing a bit more names as @tlode says, some propositions, for the setter:

markAsModified() // touch()
markAsUpdated() // touch()
markAsNew() // Set as none
resetStatus() // Set as ok
touch() // The actual

and some other for the getter:

isModified() + isNew() // Pretty clear
// Actually don't have any others, those two are enough, and we can't use getX

[stood]

getStatusEntity(), setStatusEntity() and add reserved words ?

[pounard]

It would potentially make a few developers angry to reserve keywords on entity property names, I'm not sure this would be the direction to go to.

[mvrhov]

@mvrhov ducks pommGetXXX, pommSetXXX

[tlode]

Or put those things in a separate state object, for example

$entity->meta()->touch();
$entity->meta()->isModified();
$entity->meta()->setStatus();

[chanmix51]

That sounds great 👍

[mvrhov]

Having another instance for each entity instance might be too expensive when doing bulk processing.

[tlode]

@mvrhov is right, that would be a bit of an overhead.

It depends on the implementation. If a meta instance could be shared between entities, adding a reference wouldn't be so much overhead. If meta also could be connected with the IdentityMapper, we would benefit from it, for example, by gaining control over cached in-memory instances. This might be interesting, especially for doing bulk processing.