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]

:+1:

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 :open_hands:

[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 :+1:

[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.

[chanmix51]

Bulk processing is a limitation of any OO entity manager, converting values and hydrating instances is more expensive than updating meta informations imho. Furthermore, the identitymapper is one of the largest limitations for bulk processing since each instance is kept in memory. There should be another model manager for large data processes, there is a pooler prototype on my laptop for that :)

What’s Tobias’ idea pops in my mind is that entities might embed the identitymapper. So each instance would:

• know about all the other entities exchanged with the database.
• be able to link to other entities.

Let’s bury this idea in our mind for now and see if it seeds something interesting.

Keep it simple and stupid (c) for entities meta informations.