-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enh Introduce Statable interfaces and implementation #6430
base: develop
Are you sure you want to change the base?
Conversation
332011d
to
bc8392e
Compare
Added the following section to the PR's description:
|
bc8392e
to
ed0fad2
Compare
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 Examples:
|
5e45072
to
332d6de
Compare
There is some limitation to the last use case. See |
@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! |
@martin-rueegg I'll try to give you some initial feedback for this PR this week. |
Thank you, @luke-, much appreciated. Ok, in that case I start working on a plan B ... :-) |
757049d
to
2f429b5
Compare
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.
|
Sure, I appreciate that!
Awesome!
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.
Yes, I was also wondering if moving them to the 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,
The refactoring stems mainly from the naming differences between 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
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.
Well, While Also here, I do not like that the second parameter to the above functions is defined
Yes, happy to follow your suggestion here. |
Ok.
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 :-)
Yes, it is really a pity that we use
Sounds good.
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 haven't worked much with traits yet, but shouldn't one interface be enough? The 3 relevant classes,
Would be good. I would prefer when the
I don't know if I like to override e.g. Ideally, I would stay as close to the Yii2 framework as possible here.
|
2f429b5
to
117fbef
Compare
Done.
Done. With backwards-compatible STATUS_* variants (marked deprecated).
So the
I guess bitmaks can still be a future feature. Maybe we could consider a migration of the state numeric values to
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
Done.
Makes sense. They are now also removed from the interface.
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! |
117fbef
to
0367406
Compare
Thanks!
I would prefer the second option. Migration will be necessary here, but it should be manageable.
e.g. something like this?
Ok, I understand your point. Let me think about it. But this is more a detail...
|
/** | ||
* @inheritdoc | ||
*/ | ||
public function canChangeState($state, array $options = []): bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/** | ||
* @inheritdoc | ||
*/ | ||
public function validateStateAttribute(string $attribute, $params, InlineValidator $validator, $current): bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a shortcut. It is called from
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
0367406
to
1f2101d
Compare
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?
How about:
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. |
76e6bba
to
e67a9f6
Compare
b308f52
to
b96eac1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
97ce18c
to
051a607
Compare
aa2ef0c
to
a2acc88
Compare
a2acc88
to
8542a39
Compare
8542a39
to
12c3e1c
Compare
3bfff62
to
75a77e9
Compare
@luke-, this approach is based on the pre-existing |
d149d27
to
35ce088
Compare
@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. |
35ce088
to
241fac0
Compare
Thank you for your patience. So first things first:
Here are the points where I see difficulties or have concerns.
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:
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." |
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 staticStatableInterface::getStateServiceTemplate()
the correspondingStateService
is retrieved.(This has been inspired by the implementation in v1.14.)
The
StateService
basically handlesUserStateService
, which can be extended through the eventStateServiceInterface::EVENT_INIT_STATES
state
orstatus
in the core, but leave this parameter to be adjusted if a module requires a different field name)StateServiceInterface::initStates()
)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, theStateServiceInterface::initStates()
is executed. Later on, this instance is cloned if used for a specific record. Doing so, theStateServiceInterface::EVENT_INIT_STATES
is triggered only once per StateService, rather than for every record in a recordset.However, when "attached" to a specific record, then the
StateServiceInterface::setRecord()
is called an the eventStateServiceInterface::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 theStatableQueryInterface::andWhereState($state)
method which allows easy filtering for additional return states (orStatableQueryInterface::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?
Does this PR introduce a breaking change?
The PR fulfills these requirements:
develop
branchIf adding a new feature, the PR's description includes:
Other information: