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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

timmillwood’s picture

Title: Review content_moderation moderation and mark code with @internal and @api where necessary » Review content_moderation module and mark code with @internal and @api where necessary
Issue tags: +Workflow Initiative
Sam152’s picture

Wim Leers’s picture

+1 for principle.

Sam152’s picture

Status: Active » Needs review
FileSize
2.26 KB

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

Sam152’s picture

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

plach’s picture

  1. +++ b/core/modules/content_moderation/src/ModerationInformation.php
    @@ -10,6 +10,8 @@
    + * @api
    
    +++ b/core/modules/content_moderation/src/StateTransitionValidation.php
    @@ -8,6 +8,8 @@
    + * @api
    

    Why are we making also the implementations a public @api? Are these expected to be heavily subclassed?

  2. +++ b/core/modules/content_moderation/src/ModerationInformationInterface.php
    @@ -8,6 +8,8 @@
    + * @api
    

    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.

timmillwood’s picture

I see @api as denoting things that can be used, rather than things than can be extended.

plach’s picture

Well, 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.

Sam152’s picture

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

Sam152’s picture

Issue summary: View changes
Sam152’s picture

Issue summary: View changes
timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

I think this looks ready, and something we need in before Content Moderation is marked stable.

timmillwood’s picture

Gábor Hojtsy’s picture

I think this would be best to get reviewed by a framework manager.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs framework manager review

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

Sam152’s picture

I 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

PHP and JavaScript classes
...In general, only interfaces can be relied on as the public API....

ModerationHandlerInterface.php

Entity handlers
...The interfaces which define those entity handlers though are part of the supported API

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.

timmillwood’s picture

Title: Review content_moderation module and mark code with @internal and @api where necessary » Review content_moderation module and mark code with @internal where necessary
Status: Needs work » Needs review
FileSize
2.19 KB
5.06 KB

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

Sam152’s picture

Status: Needs review » Reviewed & tested by the community

It'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.

plach’s picture

Priority: Normal » Major
Issue tags: +WI critical
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed b77f378 and pushed to 8.4.x. Thanks!

  • catch committed b77f378 on 8.4.x
    Issue #2876419 by Sam152, timmillwood, plach: Review content_moderation...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.