Problem/Motivation
The entity type provided by the content moderation module only exists because this allows the module to be installed and uninstalled. In order to access it and interact with it you should only access it via the entity it is moderating. We should review the module and decide which parts are API and which are not meant to be accessed on extended outside of core.
This issue was suggested by @dawehner after talking about #2779931: Add storage exception that enforces unique content_entity_type_id and content_entity_id on the content moderation state content entity, and add access control handler to forbid all access
Proposed resolution
Class/file | Notes |
---|---|
./src/Access/LatestRevisionCheck.php | Implicit @internal |
./src/ContentModerationState.php | @api, you’ll need to use this any time you get a state from the workflow type. |
./src/ContentModerationStateAccessControlHandler.php | Implicit @internal |
./src/ContentModerationStateInterface.php | @internal, interface for the @internal entity type |
./src/ContentModerationStateStorageSchema.php | Implicit @internal |
./src/ContentPreprocess.php | @internal, but quite ugly. Would nice to be rid of this in #2877924: Make node aware of the latest_version route. |
./src/Entity/ContentModerationState.php | @internal as discussed |
./src/Entity/Handler/BlockContentModerationHandler.php | Implicit @internal |
./src/Entity/Handler/ModerationHandler.php | Implicit @internal |
./src/Entity/Handler/ModerationHandlerInterface.php | @api |
./src/Entity/Handler/NodeModerationHandler.php | Implicit @internal |
./src/EntityOperations.php | @internal, just hook implementations |
./src/EntityTypeInfo.php | @internal, just hook implementations |
./src/Form/EntityModerationForm.php | Implicit @internal |
./src/Form/WorkflowTypeEditForm.php | Implicit @internal |
./src/ModerationInformation.php | Not specified. Interface is @internal. |
./src/ModerationInformationInterface.php | @api |
./src/ParamConverter/EntityRevisionConverter.php | Implicit @internal |
./src/Permissions.php | @internal |
./src/Plugin/Action/ModerationOptOutPublishNode.php | Implicit @internal |
./src/Plugin/Action/ModerationOptOutUnpublishNode.php | Implicit @internal |
./src/Plugin/Derivative/DynamicLocalTasks.php | Implicit @internal |
./src/Plugin/Field/FieldFormatter/ContentModerationStateFormatter.php | Implicit @internal |
./src/Plugin/Field/FieldWidget/ModerationStateWidget.php | Implicit @internal |
./src/Plugin/Field/ModerationStateFieldItemList.php | Implicit @internal |
./src/Plugin/Validation/Constraint/ModerationStateConstraint.php | Implicit @internal |
./src/Plugin/Validation/Constraint/ModerationStateConstraintValidator.php | Implicit @internal |
./src/Plugin/views/filter/LatestRevision.php | Implicit @internal |
./src/Plugin/WorkflowType/ContentModeration.php | Implicit @internal, but I think it should be @api given it’s a of a special plugin type you have to get an instance of directly to perform CM related tasks on a workflow. Marked @api. |
./src/RevisionTracker.php | @internal, as I’m hoping this becomes a feature of entity api soon |
./src/RevisionTrackerInterface.php | @internal as above |
./src/Routing/EntityModerationRouteProvider.php | Should probably be implicitly @internal, but since it’s not mentioned in the policy, marking as such. |
./src/StateTransitionValidation.php | Seems like something people might need to use, marking @api |
./src/StateTransitionValidationInterface.php | Not specified. The interface is marked @internal. |
./src/Tests/ModerationStateTestBase.php | Can probably just be removed now? #2878166: Remove deprecated ModerationStateTestBase from core |
./src/ViewsData.php | @internal, just a hook implementation |
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#18 | 2876419-18.patch | 5.06 KB | timmillwood |
#18 | interdiff-2876419-18.txt | 2.19 KB | timmillwood |
Comments
Comment #2
timmillwoodComment #3
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #4
Wim Leers+1 for principle.
Comment #5
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedMay as well kick this off. A lot of the module seems to already be implicitly @internal as per the BC policy document. It doesn't say what is implicitly @api though, looks like that discussion is still in progress. I think for now we could assume services fall under that umbrella?
I can't find any instances of an entity type being marked internal or any docs on how we'd approach communicating this. I'm presuming if the entity itself is marked internal, so are all the entity lifecycle hooks, associated APIs or just any way you would encounter or interact with the entity. I'd almost be keen on looking at reducing the surface area this actually is, can we disable non-essential hooks for example?
Other things that should obviously be internal: the interface for the entity type and the classes used for hook implementations.
Comment #6
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedList of the files as of today with notes and accompanying patch. I think the things that are implicitly internal as per the BC policy should just be left as they are. I think marking them @internal would send the message that they would have been an API otherwise, which is wrong and teaches bad habits for working with other modules.
Opening and looking at all the files has given me the sense that there are quite a few docs improvements and other minor clean ups that can be made.
Comment #7
plachWhy are we making also the implementations a public @api? Are these expected to be heavily subclassed?
We still have an instance of the "forward revision" terminology here. So I guess this is postponed on #2890364: Replace all uses of "forward revision" with "pending revision", unless we are sure that will land before 8.4.0.
Comment #8
timmillwoodI see @api as denoting things that can be used, rather than things than can be extended.
Comment #9
plachWell, we are talking about two (swappable) services, so unless people are relying on those specific implementations to subclass them, having the interfaces marked as
@api
is definitely enough. When in doubt we should pick the stricter option and possibly publish more APIs with time, the opposite wouldn't be possible without taking BC issues into account.Comment #10
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI see, so @api implies the concrete protected methods are also part of the API? If that's the case, I agree the interfaces are plenty.
Going to add the table to the IS and update.
Comment #11
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #12
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #13
timmillwoodI think this looks ready, and something we need in before Content Moderation is marked stable.
Comment #14
timmillwoodAdded this as a must have in #2755073: WI: Content Moderation module roadmap
Comment #15
Gábor HojtsyI think this would be best to get reviewed by a framework manager.
Comment #16
catchhmm I don't think we should mark anything @api here, that completely locks things down and we might regret it later, whereas the bc policy gives us flexibility or not for untagged things depending on what they are. We could open a new issue to mark things @api once workflow is stable though.
Marking things @internal is great though.
Comment #17
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI put a table in the issue summary cross referencing what each type of class was with the BC policy. I tried to only mark something as @api if the policy already enforced it, or there was some other good reason to do so. One thing I did miss though was the difference between undocumented and @api, "we'll try very hard not to break it" vs "we will try as hard as possible to not change this".
Justifications for the existing @api tagged classes/interfaces:
ModerationInformationInterface.php
StateTransitionValidationInterface.php
ModerationHandlerInterface.php
WorkflowType/ContentModeration.php
We should either add an interface for this plugin or mark this plugin @api. It's the way to accomplish important actions for content moderation such as adding an entity/bundle to a workflow (see ContentModeration::addEntityTypeAndBundle). Unless we aren't supporting an API for something so fundamental, something has gotta change here.
I think the only one I really wouldn't care losing the @api status is StateTransitionValidationInterface.php, the rest I think have fairly compelling reasons to mark @api.
Comment #18
timmillwoodI agree with @Sam152 that there are many compelling reasons to mark things as @api, although I also agree with @catch.
Lets get the @internal ones in first, and come back to @api.
Comment #19
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedIt's important implementation details like the ContentModerationState entity and the revision tracker are internal, so if @api needs more discussion, happy to make the rest of it @internal in the meantime.
Comment #20
plachComment #21
catchCommitted b77f378 and pushed to 8.4.x. Thanks!