Define the status property for the core user and comment entity

I see, however usually we just use 'boolean' as type (e.g. see node status). Any cause to stay with the integer?

It should use the correct and actual type; i.e., integer. While core merely uses 0 and 1, there are contributed modules that are setting negative integer values as well as larger positive values. Admittedly, re-using and hi-jacking a core property in that was is not recommended, but AFAIK there are modules that do this.

hm, I'm still unsure about that.

>Admittedly, re-using and hi-jacking a core property in that was is not recommended, but AFAIK there are modules that do this.
So they set negative or higher values, but still the value will be treated as boolean on/off - right? If so, we should add property info that reflects that and tells us it is boolean.

Even if modules use it and treat it as more than just on/off, I'm not sure we want to support that ?
Just for interest, which module does that?

Beside that, we also need to add in proper info about access permissions as in #1046270: User property permissions. Sry for overlooking this in #2. Probably best let's build this patch upon the solution of #1046270.

>I'm not sure which modules might be "abusing" $user->status, but I'm not really sure we can even consider this abuse.
I don't think the status column is integer to leave room for such modifications, it is because there is no boolean data type.

>I don't see why the Entity API should be enforcing a data type that is more specific than the one actually used in the database.
Indeed, it's not the task of the entity API to so. Usually modules should provide property info for their own properties, but as you know for core things are different. Anyway, core treats the status as boolean and even describes all possible values in the column description -> Whether the user is active(1) or blocked(0).. Given that description

Thus, I'd argue any module extending that to more than those boolean value is using it in an unexpected way, which we should not support. Also I think we should keep it consistent with the node.status for which have a boolean too, and hook_schema() makes it even more clear in that case:

      'status' => array(
        'description' => 'Boolean indicating whether the node is published (visible to non-administrators).',
        'type' => 'int',
        'not null' => TRUE,
        'default' => 1,
      ),

But reading the user.status description makes me thing adding an options list would be nice though, as right "status: on/off" does indeed not mean much. In Rules we could use the options list to present radio buttons instead of single check-box then. Thoughts?

>It was technically an integer, but it was represented as a decimal and so caused Entity to break on set().
Hm, if you alter the property info to decimal, that should not happen?

>I'm not sure I understand what needs to happen for the user access properties... we have an access callback already, it just needs to be used on more properties that were overlooked?

I think Drupal usually doesn't expose the user.status to users which are able to view user accounts of others. For reflecting that in the property info, I guess it suffices to make sure view access is restricted to admins. More important we need do restrict write access to admins too.

I'd argue any module extending that to more than those boolean value is using it in an unexpected way, which we should not support

I don't think it's Entity API's responsibility to decide on this. AFAICS, we're talking about describing an entity's properties here, and that's it. Hence, Entity API should make sure to describe entity properties in the actual and proper data types. Regardless of whether the data type is considered, expected, or interpreted in any special way at the application-level. Decisions on the application-level can freely change without notice, possibly keeping the existing data type. However, application-level logic may needlessly break if there is a storage/meta layer in between that added its own interpretation of properties in between.

IIRC, though this is based on very old threads, Drupal core intentionally left room for different values having the same effective meaning for core functionality, but allowing contributed modules to plug additional behavior/logic into the 'status'.

I'd highly recommend to go with the actual data type (integers; possibly even signed). It's just simply not Entity API's job to enforce interpretation of those integers as Booleans, IMHO.

IIRC, though this is based on very old threads, Drupal core intentionally left room for different values having the same effective meaning for core functionality, but allowing contributed modules to plug additional behavior/logic into the 'status'.

I guess that was the initial intention when 'status' has been created, although I can't really think how this should work cleanly - whatever.

I'd highly recommend to go with the actual data type (integers; possibly even signed). It's just simply not Entity API's job to enforce interpretation of those integers as Booleans, IMHO.

Indeed, so let's do it that way and just go with an integer type + an options list for the possible values.

The odd thing here is that hook_schema() has no booleans, so the entity API cannot really know whether it should be boolean or not.
So what about node.status then? Should we migrate it to an integer too although the description states it is boolean? I must say that 'status' with a list of possible states (-> options list) makes more sense than a boolean status (yes / no).

Ok, I guess we need a decision here to move on - so let's go with 'integer' + an options list for the possible values for all status properties.

Here's a patch based on decision from #10

as always, typo :)

better title

Thanks. We should also fix node.status to work the same way + be an integer though. Then we should make complete property definitions, i.e. that stuff node.status has is missing for user.status and comment.status:

    'setter permission' => 'administer nodes',
    'schema field' => 'status',

- in particular we need to fix access. If the permission does not suffice, we can provide an 'access callback' too.

ok, I've done so and committed it.