Enh Introduce Statable interfaces and implementation

[martin-rueegg]

The main idea of this PRis to standardize the way state/status fields are implemented:

• The record implements StatableInterface

• Through the record's instance's StatableInterface->getStateService() or the static StatableInterface::getStateServiceTemplate() the corresponding StateService is retrieved.
  (This has been inspired by the implementation in v1.14.)

• The StateService basically handles

  • the available states for the given record type (eg. see UserStateService, which can be extended through the event StateServiceInterface::EVENT_INIT_STATES
  • the state field name of the given table (there is some inconsistency here, I guess we should either use state or status in the core, but leave this parameter to be adjusted if a module requires a different field name)
  • the default filtering states when retrieving such records (also initialized in StateServiceInterface::initStates())
  • validating states
  • updating the state on the record

• The current implementation also supports the use of $stateFilterCondition (as introduced in v1.14) as it allows to make certain states being dependent on another criteria. Otherwise, this functionality could be dropped.)

Initializing the available states and the default filtered states

Generally speaking, one instance of a given StateService is initialized upon first access (be it statically or on the instance). At this point, the StateServiceInterface::initStates() is executed. Later on, this instance is cloned if used for a specific record. Doing so, the StateServiceInterface::EVENT_INIT_STATES is triggered only once per StateService, rather than for every record in a recordset.

/**
 * Function is called upon object initialisation (static::init()) and
 * 1. SHOULD set up the allowed states
 * 2. CAN define the default queried states
 * 3. MUST call the static::EVENT_INIT_STATES event
 *
 * @see static::EVENT_INIT_STATES
 */
public function initStates(): StateServiceInterface;

However, when "attached" to a specific record, then the StateServiceInterface::setRecord() is called an the event StateServiceInterface::EVENT_SET_RECORD is issued instead. This allows to adjust the StateService based upon the individual record, if required.

Filtering records

The find() method of a statable class SHOULD return a query implementing the StatableActiveQueryInterface (or at least the StatableQueryInterface). This query provides the StatableQueryInterface::andWhereState($state) method which allows easy filtering for additional return states (or StatableQueryInterface::whereState($state) to specify the exact set of returned states, ignoring the defaults).

These states will be translated into an SQL where clause during the ActiveQueryInterface::prepare() function, and hence is applied automagically.

PR-Admin

What kind of change does this PR introduce?

• Refactor/new feature

Does this PR introduce a breaking change?

• No, should not

The PR fulfills these requirements:

• It's submitted to the develop branch
• See Enh: [RFC] [WIP] Make profile image handling generic and re-usable for any content #6408
• All tests are passing
• New/updated tests are included
• Changelog was modified

If adding a new feature, the PR's description includes:

• See Enh: [RFC] [WIP] Make profile image handling generic and re-usable for any content #6408

Other information:

[martin-rueegg]

Added the following section to the PR's description:

• Initializing the available states and the default filtered states
• Filtering records

[martin-rueegg]

As it appears, the use case to filter on the state in combination with another field is more common than I first thought:

In a (wall)stream, users can see their own drafts and scheduled posts.

This could be easy implemented by passing the state as an condition-array with the state as the key and the linked condition as the value (again, as an array). This could then be translated by the andWhereState() method into the $stateFilterCondition.

Examples:

• andWhereState(100) to include deleted records
• andWhereState([10, 100]) to include drafts and deleted records
• andWhereState([10 => ['user.ID' => $user]]) to include the drafts of $user (added as an AND condition for this specific state)

[martin-rueegg]

As it appears, the use case to filter on the state in combination with another field is more common than I first thought:

In a (wall)stream, users can see their own drafts and scheduled posts.

This could be easy implemented by passing the state as an condition-array with the state as the key and the linked condition as the value (again, as an array). This could then be translated by the andWhereState() method into the $stateFilterCondition.

Examples:

• andWhereState(100) to include deleted records
• andWhereState([10, 100]) to include drafts and deleted records
• andWhereState([10 => ['user.ID' => $user]]) to include the drafts of $user (added as an AND condition for this specific state)

There is some limitation to the last use case. See StateServiceInterface::validateState() for the currently proposed implementation and StateServiceTest for examples.

[martin-rueegg]

@luke- do you have a rough idea when you could spare some time to have a look at this PR and later on the related follow-ups?

Since our module is relying on this code, we would have to find another solution in case this is not feasible in the near future are is not accepted at all.

Thank you!

[luke-]

@martin-rueegg I'll try to give you some initial feedback for this PR this week.
If everything works as planned, the PR can be integrated into the stable version 1.16 (beta planned in November '23).

[martin-rueegg]

@martin-rueegg I'll try to give you some initial feedback for this PR this week. If everything works as planned, the PR can be integrated into the stable version 1.16 (beta planned in November '23).

Thank you, @luke-, much appreciated.

Ok, in that case I start working on a plan B ... :-)

[luke-]

@martin-rueegg I'll try to give you some initial feedback for this PR this week. If everything works as planned, the PR can be integrated into the stable version 1.16 (beta planned in November '23).

Thank you, @luke-, much appreciated.

Ok, in that case I start working on a plan B ... :-)

Thank you for your patience. When it is time-critical, a plan B is probably good, especially such core changes that also require refactoring of modules always take some extra time.

But anyway, here's my promised feedback.

I really like the refactoring of the State/Status part.

Especially the unification (Content, Spaces, Users) of the services and ActiveQuery classes makes really sense.

• Status Constants: I'm a bit unsure if it's the best way to have all state constants in the StatableInterface::STATE_*. In fact, almost all of them can be used in several core modules, but there are also some like STATE_DRAFT which is only useful in the Content module.
  Any idea about this? Maybe we could move them in the Service class or leave them in the model (reduce refactoring)?
  This requires a larger refactoring (Remove User::STATUS_ , Content::STATE_, Space::STATUS_) - maybe it makes sense to wait for PHP 8.1 and Enums?
  With this migration it would also make sense to unify status values, e.g. Content::STATE_SOFT_DELETED and User::STATUS_DELETED. But this would require some extra DB migrations.
• Modules API: If I understand it correctly, methods like StatableTrait::find*() are not used in the core and are rather available for modules. Although it is surely useful for modules, I would like to avoid such code that is not used actively in the core itself. Of course, there are exceptions here, but I want to keep the complexity here as low as possible.
• Class Naming: I would like to get rid of/reduce the humhub\libs namespace in the long run. Maybe humhub\services\AbstractStateService, humhub\services\stateable\DeletableTrait. StateableActive* could be moved to components. Just as an idea, am still a bit unsure.

[martin-rueegg]

(...) especially such core changes that also require refactoring of modules always take some extra time.

Sure, I appreciate that!

But anyway, here's my promised feedback.
I really like the refactoring of the State/Status part.
Especially the unification (Content, Spaces, Users) of the services and ActiveQuery classes makes really sense.

Awesome!

• Status Constants: I'm a bit unsure if it's the best way to have all state constants in the StatableInterface::STATE_*. In fact, almost all of them can be used in several core modules, but there are also some like STATE_DRAFT which is only useful in the Content module.

The thought behind this was discussed in a forum discussion. Hence, my suggestion would be, that only those values, that are reserved for humhub (Byte 1, bits 1-8, or values 1-32,767) would be manage in the interface. Maybe, this could be further split into "core states" and "module states" where only the core states (e.g. bits 1-4, or values 1-16'000-something) would sit in the interface, and the module values could extend that interface and define there values there.

Any idea about this? Maybe we could move them in the Service class or leave them in the model (reduce refactoring)?

Yes, I was also wondering if moving them to the StateServiceInterface would make more sense. But then I thought, the state is actually a property of the record (hence, the StatableInterface is set on the model). Whereas the StateService is, as it's name suggests, a service that deals with the record and it's properties. It is not fully coherent, as the allowed and default states are actually defined/managed in the service.

Moving them from the model to the interface does not really make a difference, as the model inherits the constant from the interface anyway. So, for example, User::STATE_DELETED can be used anywhere, as User implements StatableInterface where StatableInterface::STATE_DELETED is defined. I only did update the name in other places for consistency, but it would not be required.

This requires a larger refactoring (Remove User::STATUS_ , Content::STATE_, Space::STATUS_) - maybe it makes sense to wait for PHP 8.1 and Enums?

The refactoring stems mainly from the naming differences between STATUS_ and STATE_, as for the rest both the model class or the interface can be used interchangeably. But I'd suggest to streamline the naming convention to either use state (which allows plural "states" and is hence more "common") or status (it's plural is still status, but less naming conflict with state, as in the sense of a country) in the code as well as the column name. It is a bit unfortunate, that this has not been considered when introducing the states for users, spaces, and content ...

However, as far as the code is concerned, we could chose one term (state or status) and then create Constants in the model for backward-compatibility, marking them deprecated, and pointing to the constant in the Interface. To unify the access to the record property, we could also make a stub getState/setState function, if we'd choose "status" as the term but the field "status" is used in the db-table.

With this migration it would also make sense to unify status values, e.g. Content::STATE_SOFT_DELETED and User::STATUS_DELETED. But this would require some extra DB migrations.

Well, yes, if these are not really different states, they SHOULD be unified indeed. Maybe some reflection should be done on the possibility of using the state as a mask. E.g. can it be DELETED and ARCHIVED at the same time.

• Modules API: If I understand it correctly, methods like StatableTrait::find*() are not used in the core and are rather available for modules.

Well, StatableTrait::find() is used all over the code, also in the core. It ensures that the the returned query is a StatableActiveQuery.

While findOne, findAll (and hence findByCondition) are not used in the current PR. However, they are used also in the core in case the follow-up PRs would be implemented as well. So for now, they could be removed from the trait and interface and be re-added in the PR they are first used.

Also here, I do not like that the second parameter to the above functions is defined $allowedStates = null. I guess a better (and more future-proof approach) would be to define it as array $params = [] where the key allowedStates would be reserved and used.

• Class Naming: I would like to get rid of/reduce the humhub\libs namespace in the long run. Maybe humhub\services\AbstractStateService, humhub\services\stateable\DeletableTrait. StateableActive* could be moved to components. Just as an idea, am still a bit unsure.

Yes, happy to follow your suggestion here.

[luke-]

(...) especially such core changes that also require refactoring of modules always take some extra time.

Sure, I appreciate that!

But anyway, here's my promised feedback.
I really like the refactoring of the State/Status part.
Especially the unification (Content, Spaces, Users) of the services and ActiveQuery classes makes really sense.

Awesome!

• Status Constants: I'm a bit unsure if it's the best way to have all state constants in the StatableInterface::STATE_*. In fact, almost all of them can be used in several core modules, but there are also some like STATE_DRAFT which is only useful in the Content module.

The thought behind this was discussed in a forum discussion. Hence, my suggestion would be, that only those values, that are reserved for humhub (Byte 1, bits 1-8, or values 1-32,767) would be manage in the interface. Maybe, this could be further split into "core states" and "module states" where only the core states (e.g. bits 1-4, or values 1-16'000-something) would sit in the interface, and the module values could extend that interface and define there values there.

Ok.

Any idea about this? Maybe we could move them in the Service class or leave them in the model (reduce refactoring)?

Yes, I was also wondering if moving them to the StateServiceInterface would make more sense. But then I thought, the state is actually a property of the record (hence, the StatableInterface is set on the model). Whereas the StateService is, as it's name suggests, a service that deals with the record and it's properties. It is not fully coherent, as the allowed and default states are actually defined/managed in the service.

Moving them from the model to the interface does not really make a difference, as the model inherits the constant from the interface anyway. So, for example, User::STATE_DELETED can be used anywhere, as User implements StatableInterface where StatableInterface::STATE_DELETED is defined. I only did update the name in other places for consistency, but it would not be required.

Ok, thats nice of course. I've missed that fact.

For the PR review it would have been easier to do this renaming at a later time or even separately, then the overview of actual PR changes would be better. e.g. instead of 80 changed files ~20 :-)

This requires a larger refactoring (Remove User::STATUS_ , Content::STATE_, Space::STATUS_) - maybe it makes sense to wait for PHP 8.1 and Enums?

The refactoring stems mainly from the naming differences between STATUS_ and STATE_, as for the rest both the model class or the interface can be used interchangeably. But I'd suggest to streamline the naming convention to either use state (which allows plural "states" and is hence more "common") or status (it's plural is still status, but less naming conflict with state, as in the sense of a country) in the code as well as the column name. It is a bit unfortunate, that this has not been considered when introducing the states for users, spaces, and content ...

Yes, it is really a pity that we use STATE and STATUS and do not use a unified name here.
I just searched the modules repos. Probably it would be a smaller effort to rename User|Space::STATUS to STATE.

However, as far as the code is concerned, we could chose one term (state or status) and then create Constants in the model for backward-compatibility, marking them deprecated, and pointing to the constant in the Interface. To unify the access to the record property, we could also make a stub getState/setState function, if we'd choose "status" as the term but the field "status" is used in the db-table.

Sounds good.

With this migration it would also make sense to unify status values, e.g. Content::STATE_SOFT_DELETED and User::STATUS_DELETED. But this would require some extra DB migrations.

Well, yes, if these are not really different states, they SHOULD be unified indeed. Maybe some reflection should be done on the possibility of using the state as a mask. E.g. can it be DELETED and ARCHIVED at the same time.

A bitmask would of course be very elegant. I haven't worked with it for a while. But I'm afraid it would make the PR and any changes even more complex. If several STATEs are possible. Currently, for example, we had a problem determining whether a piece of content had ever been published. This could have been handled with a mask very well. (But we have found another solution).

• Modules API: If I understand it correctly, methods like StatableTrait::find*() are not used in the core and are rather available for modules.

Well, StatableTrait::find() is used all over the code, also in the core. It ensures that the the returned query is a StatableActiveQuery.

I haven't worked much with traits yet, but shouldn't one interface be enough? The 3 relevant classes, User::find() : ActiveQueryUser, Space::find() : ActiveQuerySpace and Content::find() : ActiveQueryContent override the find() method anyway.

While findOne, findAll (and hence findByCondition) are not used in the current PR. However, they are used also in the core in case the follow-up PRs would be implemented as well. So for now, they could be removed from the trait and interface and be re-added in the PR they are first used.

Would be good. I would prefer when the find*() methods and other methods that may be needed later by modules or other PRs are not included in this PR for now.

Also here, I do not like that the second parameter to the above functions is defined $allowedStates = null. I guess a better (and more future-proof approach) would be to define it as array $params = [] where the key allowedStates would be reserved and used.

I don't know if I like to override e.g. findOne() with an additional parameter.

Ideally, findOne(), findAll(), ... always work with defaults (e.g. Published State) which the respective ActiveQuery uses. If you want special states you have to work with find().

I would stay as close to the Yii2 framework as possible here.

• Class Naming: I would like to get rid of/reduce the humhub\libs namespace in the long run. Maybe humhub\services\AbstractStateService, humhub\services\stateable\DeletableTrait. StateableActive* could be moved to components. Just as an idea, am still a bit unsure.

Yes, happy to follow your suggestion here.

[martin-rueegg]

Moving them from the model to the interface does not really make a difference, as the model inherits the constant from the interface anyway. So, for example, User::STATE_DELETED can be used anywhere, as User implements StatableInterface where StatableInterface::STATE_DELETED is defined. I only did update the name in other places for consistency, but it would not be required.

Ok, thats nice of course. I've missed that fact.

For the PR review it would have been easier to do this renaming at a later time or even separately, then the overview of actual PR changes would be better. e.g. instead of 80 changed files ~20 :-)

Done.

This requires a larger refactoring (Remove User::STATUS_ , Content::STATE_, Space::STATUS_) - maybe it makes sense to wait for PHP 8.1 and Enums?

The refactoring stems mainly from the naming differences between STATUS_ and STATE_, as for the rest both the model class or the interface can be used interchangeably. But I'd suggest to streamline the naming convention to either use state (which allows plural "states" and is hence more "common") or status (it's plural is still status, but less naming conflict with state, as in the sense of a country) in the code as well as the column name. It is a bit unfortunate, that this has not been considered when introducing the states for users, spaces, and content ...

Yes, it is really a pity that we use STATE and STATUS and do not use a unified name here. I just searched the modules repos. Probably it would be a smaller effort to rename User|Space::STATUS to STATE

Done. With backwards-compatible STATUS_* variants (marked deprecated).

(...) To unify the access to the record property, we could also make a stub getState/setState function, if we'd choose "status" as the term but the field "status" is used in the db-table.

Sounds good.

So the user and space tables use the field name status. Now we have two options:

1. keep the field name and add getState/setState methods to the User and Space models for code-API consistency

• No issue with backward-compatibility
• No consistent naming convention on table-level

2. rename the field and add getStatus/setStatus methods (marked deprecated) to the User and Space models for code-API consistency

• Breaks SQL-level backward-compatibility. However, since the feature is realatively new, this might be a reasonable trade-off?
• Full consistency in terms of record state implementation

With this migration it would also make sense to unify status values, e.g. Content::STATE_SOFT_DELETED and User::STATUS_DELETED. But this would require some extra DB migrations.

Well, yes, if these are not really different states, they SHOULD be unified indeed. Maybe some reflection should be done on the possibility of using the state as a mask. E.g. can it be DELETED and ARCHIVED at the same time.

A bitmask would of course be very elegant. I haven't worked with it for a while. But I'm afraid it would make the PR and any changes even more complex. If several STATEs are possible. Currently, for example, we had a problem determining whether a piece of content had ever been published. This could have been handled with a mask very well. (But we have found another solution).

I guess bitmaks can still be a future feature. Maybe we could consider a migration of the state numeric values to
a. unify them
b. having bitmask use in mind for the future

• Modules API: If I understand it correctly, methods like StatableTrait::find*() are not used in the core and are rather available for modules.

Well, StatableTrait::find() is used all over the code, also in the core. It ensures that the the returned query is a StatableActiveQuery.

I haven't worked much with traits yet, but shouldn't one interface be enough? The 3 relevant classes, User::find() : ActiveQueryUser, Space::find() : ActiveQuerySpace and Content::find() : ActiveQueryContent override the find() method anyway.

I tend to create a trait alongside an interface with a standard-implementation of the required functions. As such, to add the functionality of the interface, one more line (use trait) may be enough in most cases.

You are correct, the StatableTrait::find() is overridden by User, Space, and ContentActiveRecord classes, but not by the Content class. Since the method is used and a very generic default, I'd leave it in the trait, rather than moving it to the Content class and running the risk having to implement it in other classes as well. Also, since most records derive from the DbActiveRecord, which also implements the function, there is a risk of forgetting the implementation. Unless we add an abstract method to the trait. But then again we can as well have the default implementation.

While findOne, findAll (and hence findByCondition) are not used in the current PR. However, they are used also in the core in case the follow-up PRs would be implemented as well. So for now, they could be removed from the trait and interface and be re-added in the PR they are first used.

Would be good. I would prefer when the find*() methods and other methods that may be needed later by modules or other PRs are not included in this PR for now.

Done.

Also here, I do not like that the second parameter to the above functions is defined $allowedStates = null. I guess a better (and more future-proof approach) would be to define it as array $params = [] where the key allowedStates would be reserved and used.

I don't know if I like to override e.g. findOne() with an additional parameter.

Ideally, findOne(), findAll(), ... always work with defaults (e.g. Published State) which the respective ActiveQuery uses. If you want special states you have to work with find().

Makes sense. They are now also removed from the interface.

• Class Naming: I would like to get rid of/reduce the humhub\libs namespace in the long run. Maybe humhub\services\AbstractStateService, humhub\services\stateable\DeletableTrait. StateableActive* could be moved to components. Just as an idea, am still a bit unsure.

Yes, happy to follow your suggestion here.

Happy to move the Services to the components namespace. However, in some modules there is already a service namespace. Would appreciate more directions here as to what is best for you, once you know better.

Thank you!

[luke-]

Moving them from the model to the interface does not really make a difference, as the model inherits the constant from the interface anyway. So, for example, User::STATE_DELETED can be used anywhere, as User implements StatableInterface where StatableInterface::STATE_DELETED is defined. I only did update the name in other places for consistency, but it would not be required.

Ok, thats nice of course. I've missed that fact.
For the PR review it would have been easier to do this renaming at a later time or even separately, then the overview of actual PR changes would be better. e.g. instead of 80 changed files ~20 :-)

Done.

Thanks!

This requires a larger refactoring (Remove User::STATUS_ , Content::STATE_, Space::STATUS_) - maybe it makes sense to wait for PHP 8.1 and Enums?

The refactoring stems mainly from the naming differences between STATUS_ and STATE_, as for the rest both the model class or the interface can be used interchangeably. But I'd suggest to streamline the naming convention to either use state (which allows plural "states" and is hence more "common") or status (it's plural is still status, but less naming conflict with state, as in the sense of a country) in the code as well as the column name. It is a bit unfortunate, that this has not been considered when introducing the states for users, spaces, and content ...

Yes, it is really a pity that we use STATE and STATUS and do not use a unified name here. I just searched the modules repos. Probably it would be a smaller effort to rename User|Space::STATUS to STATE

Done. With backwards-compatible STATUS_* variants (marked deprecated).

(...) To unify the access to the record property, we could also make a stub getState/setState function, if we'd choose "status" as the term but the field "status" is used in the db-table.

Sounds good.

So the user and space tables use the field name status. Now we have two options:

1. keep the field name and add getState/setState methods to the User and Space models for code-API consistency

• No issue with backward-compatibility
• No consistent naming convention on table-level

2. rename the field and add getStatus/setStatus methods (marked deprecated) to the User and Space models for code-API consistency

• Breaks SQL-level backward-compatibility. However, since the feature is realatively new, this might be a reasonable trade-off?
• Full consistency in terms of record state implementation

I would prefer the second option. Migration will be necessary here, but it should be manageable.

With this migration it would also make sense to unify status values, e.g. Content::STATE_SOFT_DELETED and User::STATUS_DELETED. But this would require some extra DB migrations.

Well, yes, if these are not really different states, they SHOULD be unified indeed. Maybe some reflection should be done on the possibility of using the state as a mask. E.g. can it be DELETED and ARCHIVED at the same time.

A bitmask would of course be very elegant. I haven't worked with it for a while. But I'm afraid it would make the PR and any changes even more complex. If several STATEs are possible. Currently, for example, we had a problem determining whether a piece of content had ever been published. This could have been handled with a mask very well. (But we have found another solution).

I guess bitmaks can still be a future feature. Maybe we could consider a migration of the state numeric values to a. unify them b. having bitmask use in mind for the future

e.g. something like this?

• DISABLED 0
• ENABLED|PUBLISHED 1
• NEEDS_APPROVAL 2
• DRAFT 4
• SCHEDULED 8
• DELETED|SOFT_DELETED 128

• Modules API: If I understand it correctly, methods like StatableTrait::find*() are not used in the core and are rather available for modules.

Well, StatableTrait::find() is used all over the code, also in the core. It ensures that the the returned query is a StatableActiveQuery.

I haven't worked much with traits yet, but shouldn't one interface be enough? The 3 relevant classes, User::find() : ActiveQueryUser, Space::find() : ActiveQuerySpace and Content::find() : ActiveQueryContent override the find() method anyway.

I tend to create a trait alongside an interface with a standard-implementation of the required functions. As such, to add the functionality of the interface, one more line (use trait) may be enough in most cases.

You are correct, the StatableTrait::find() is overridden by User, Space, and ContentActiveRecord classes, but not by the Content class. Since the method is used and a very generic default, I'd leave it in the trait, rather than moving it to the Content class and running the risk having to implement it in other classes as well. Also, since most records derive from the DbActiveRecord, which also implements the function, there is a risk of forgetting the implementation. Unless we add an abstract method to the trait. But then again we can as well have the default implementation.

Ok, I understand your point. Let me think about it. But this is more a detail...

While findOne, findAll (and hence findByCondition) are not used in the current PR. However, they are used also in the core in case the follow-up PRs would be implemented as well. So for now, they could be removed from the trait and interface and be re-added in the PR they are first used.

Would be good. I would prefer when the find*() methods and other methods that may be needed later by modules or other PRs are not included in this PR for now.

Done.

Also here, I do not like that the second parameter to the above functions is defined $allowedStates = null. I guess a better (and more future-proof approach) would be to define it as array $params = [] where the key allowedStates would be reserved and used.

I don't know if I like to override e.g. findOne() with an additional parameter.
Ideally, findOne(), findAll(), ... always work with defaults (e.g. Published State) which the respective ActiveQuery uses. If you want special states you have to work with find().

Makes sense. They are now also removed from the interface.

• Class Naming: I would like to get rid of/reduce the humhub\libs namespace in the long run. Maybe humhub\services\AbstractStateService, humhub\services\stateable\DeletableTrait. StateableActive* could be moved to components. Just as an idea, am still a bit unsure.

Yes, happy to follow your suggestion here.

Happy to move the Services to the components namespace. However, in some modules there is already a service namespace. Would appreciate more directions here as to what is best for you, once you know better.

Thank you!

[luke-]

Do we really need this "shortcut"? The classes like User, Spaces, Content are currently quite big and I want try to reduce methods as possible...

[martin-rueegg]

In the light of PR #6451 I feel it does make sense to have a standardized way to check if a state is allowed or not (at this time). Hence it complements the

• ArchiveableInterface::canArchive($user = null): bool
• EditableInterface::canEdit($user = null): bool
• DeletableInterface::canDelete($user = null): bool
• ReadableInterface::canRead($user = null): bool
• ViewableInterface::canView($user = null): bool

series of methods.

[luke-]

Is this used?

[martin-rueegg]

It is a shortcut. It is called from

• StatableTrait::validateStateAttribute
• StatableQueryTrait::checkStates

The latter is used every time a StatableQueryTrait::whereState/andWhereState is called. It is also available to be used as a validator in the rules() section. This should indeed be added in the changed Models:

• User
• Space
• Membership
• Content

[martin-rueegg]

So the user and space tables use the field name status. Now we have two options:

1. keep the field name and add getState/setState methods to the User and Space models for code-API consistency

• No issue with backward-compatibility
• No consistent naming convention on table-level

2. rename the field and add getStatus/setStatus methods (marked deprecated) to the User and Space models for code-API consistency

• Breaks SQL-level backward-compatibility. However, since the feature is relatively new, this might be a reasonable trade-off?
• Full consistency in terms of record state implementation

I would prefer the second option. Migration will be necessary here, but it should be manageable.

Good choice. :-) Although I only realized now, that the status fields have been around since the beginning of the respective tables

In addition, I'd suggest to make the table field unsigned.

I have locally created a patch to rename the field. However, I was wondering when adding it to this PR, it will cause another 40 files to be changed, due to the renaming of the field in different places.

Would it make sense to create a separate branch, once this PR has been "approved" so far, then apply the PR to that new branch and then create a second PR with the field renaming, which can then be applied on top, and then both together eventually merged into develop?

With this migration it would also make sense to unify status values, e.g. Content::STATE_SOFT_DELETED and User::STATUS_DELETED. But this would require some extra DB migrations.

Well, yes, if these are not really different states, they SHOULD be unified indeed. Maybe some reflection should be done on the possibility of using the state as a mask. E.g. can it be DELETED and ARCHIVED at the same time.

A bitmask would of course be very elegant. I haven't worked with it for a while. But I'm afraid it would make the PR and any changes even more complex. If several STATEs are possible. Currently, for example, we had a problem determining whether a piece of content had ever been published. This could have been handled with a mask very well. (But we have found another solution).

I guess bitmaks can still be a future feature. Maybe we could consider a migration of the state numeric values to a. unify them b. having bitmask use in mind for the future

e.g. something like this?

* DISABLED                    0
* ENABLED|PUBLISHED           1
* NEEDS_APPROVAL              2
* DRAFT                       4
* SCHEDULED                   8
* DELETED|SOFT_DELETED        128

How about:

• 0 = DISABLED
• 1 = DRAFT
• 8 = ENABLED|PUBLISHED
• 9 = INVALID (or default state while creating the record, eg. to get a record ID that is then used for a parent/child record, since it makes no sense to have a record both enabled and in DRAFT; set as default value in SQL)
• 10 = NEEDS_APPROVAL (will be ENABLED|PUBLISHED later)
• 12 = SCHEDULED (will be PUBLISHED later)
• 128 = DELETED|SOFT_DELETED

Using a higher value for ENABLED|PUBLISHED allows to use values close to each other (n+1 - n+7) to have a "connected meaning"

I've also updated both Space and Membership to use the new interface/mechanism.

[martin-rueegg]

• requires Fix #6720 Regression re isDatabaseInstalled #6729

[martin-rueegg]

@luke-, this approach is based on the pre-existing ContentStateService from v1.15 which has been extended to be generally available. There is no default filter on the ActiveQuery, unless specified by design on a specific implementation.

[martin-rueegg]

@luke- Any chance to give this one a review? The approach has been significantly simplified and is based on code which has been introduced in v1.15.

Your time and input would be much appreciated.

[luke-]

Thank you for your patience. So first things first:

• I like the idea of introducing a unified central 'StateService' that can be overwritten by specific implementations. And all state-related things are moved here.
• I also appreciate the unification of states regarding the ActiveQuery.

Here are the points where I see difficulties or have concerns.

• I don't know if I want to allow modules to extend or heavily manipulate the state system. This results in a lot of additional complexity (code which is only used by 1-2 modules) and increased maintenance effort, e.g. in case of refactoring... I would rather include relevant use cases such as approvals directly in the core.
• I'm not sure whether it makes sense to collect all states centrally in the interface. It would be easiest if each StateService implementation maintained its own states (and also own values for it).
• I'm not a big fan of traits. Before we have 8 traits which are only shortcuts for isEnabled() === $this->is(StatableInterface::STATE_ENABLED);. I would rather remove them completely and only use the State Service methods set and is. Also a StateService could implement own related shortcuts.
• The ActiveQuery is still giving me a bit of a headache. I still need some time here. All central components (content, users, spaces, modules, file,...) will get a StateService/Query in the long term. Do I understand correctly that you are also trying to add the condition the default state of joined models that implement stateable?

Basically, the solution seems a bit too comprehensive to me. Without knowing all the exact challenges, - for me - ideally there would be only 4 classes that provide the state logic:

• AbstractStateService
• ContentStateService, ...
• ActiveQueryStateBehavior?
• StatableActiveRecordInterface

Unfortunately, I'm a bit short on time atm, but if you'd like, I can take a deeper look, maybe a implementation which few files works as well. The ActiveQuery topic is particularly interesting (and also complex). Perhaps Behaviors could be effectively used here, even though I'm only a big fan of them."