Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Follow-up for #2899553: Architectural review of the Workflows module (documentation cleanups).
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#18 | 2900421-18.patch | 24.37 KB | timmillwood |
#18 | interdiff-2900421-18.txt | 3.14 KB | timmillwood |
Comments
Comment #2
Wim LeersComment #3
timmillwoodComment #4
timmillwoodWith the beta deadline tomorrow and all "WI Critical" issues RTBC or Fixed we're in a good place, but the review of Content Moderation hasn't been done yet.
Looking at #2755073: WI: Content Moderation module roadmap there is one final "must have":
#2862041: Provide useful Views filters for Content Moderation State fields
Two "should have issues:
#2873287: Dispatch events for changing content moderation states - This isn't a major priority because hook_entity_update and hook_entity_create can be used, although not ideal.
#2890380: Remove descriptions of permissions in Content Moderation - This doesn't provide any added features, and isn't a BC break, it just brings Content Moderation in line with all other core modules.
Comment #5
larowlanAiming to have a look at this tomorrow
Comment #6
timmillwoodI've started taking a look found a couple of very minor nit picks.
Here's what I have so far:
content_moderation.module
:Not sure we need this comment
content_moderation_workflow_update()
callscontent_moderation_workflow_insert()
, this seems a bit weird.ContentModerationStateInterface.php
Drupal\content_moderation\ContentModerationStateInterface
isn't forDrupal\content_moderation\ContentModerationState
, it's forDrupal\content_moderation\Entity\ContentModerationState
. Maybe we should move the interface into the Entity directory.Comment #7
timmillwoodEntityOperations
entityPresave
is a Hook bridge.entityPresave
$entity->moderation_state->value
should never return NULL, so do we really need the check?EntityTypeInfo
EntityOperations
.Handler classes
Comment #8
tacituseu CreditAttribution: tacituseu commented1.
core/modules/content_moderation/src/EntityTypeInfo.php
contains leftover from times it was an 'entity_reference':->setSetting('target_type', 'moderation_state')
2.
core/modules/content_moderation/src/Plugin/Validation/Constraint/ModerationStateConstraintValidator.php
injectsStateTransitionValidation
but doesn't use itComment #9
timmillwoodFixing everything mentioned apart from #6
content_moderation.module
.2 because I'm not sure we really need to abstract all hooks into classes.Comment #10
timmillwoodGood spot!
Patch fixes both items in #8.
Comment #13
timmillwoodI guess
$entity->moderation_state->value
can be null.Comment #14
tim.plunkett1)
My biggest concern is the public methods called on individual implementations, with no backing interface.
For example:
Workflow::loadMultipleByType('content_moderation')
loads only workflow entities that use a plugin with the ID ofcontent_moderation
.By default, that corresponds to the
\Drupal\content_moderation\Plugin\WorkflowType\ContentModeration
class.That class has several pubic methods that are on NO interface. Two of them are called here: appliesToEntityTypeAndBundle() and removeEntityTypeAndBundle().
There is no guarantee that those methods will exist. Anyone could swap out the plugin class for "content_moderation", and have no indication that those methods must exist.
2)
The point in #6.2 about hook implementations is a good one, but not a blocker. Those should be made consistent eventually.
3)
\Drupal\content_moderation\ContentModerationState is bad, but that's Workflows' fault, see #2899553-29: Architectural review of the Workflows module (documentation cleanups)
4)
Despite the docblock in \Drupal\content_moderation\Access\LatestRevisionCheck::loadEntity() explaining why it throws an unclassed Exception, I still disagree. This should use \Drupal\Core\Access\AccessException.
5)
\Drupal\workflows\WorkflowTypeInterface::getInitialState() has no parameters.
\Drupal\content_moderation\Plugin\WorkflowType\ContentModeration::getInitialState() does. Yikes.
Sure, it has an exception is thrown.
But there is no test coverage for this?
I see a couple Unit tests, but none for this plugin.
6)
I think it's called out above, but the various entity-adjacent classes are spread out in a confusing manner.
By the namespace, I would assume \Drupal\content_moderation\Routing\EntityModerationRouteProvider was a subclass of \Drupal\Core\Routing\RouteSubscriberBase, but instead is an implementation of EntityRouteProviderInterface.
Additionally, all the classes delegated to by hooks are mostly dumped in /src/
Comment #15
timmillwoodThanks @tim.plunkett, we should be able to address most of these here.
Comment #16
timmillwood#14.1 - Added ContentModerationInterface.
#14.2 - Agreed
#14.3 - StateInterface and Transition interface are @internal, so I guess we could remove them in 8.5.x if needed.
#14.4 - Switched to AccessException.
#14.5 - Not sure what to do for the best here, I personally don't have an issue with the optional param, but one suggestion from @sam152 was to add a method such as getModerationInitialState($entity); which is just used by Content Moderation.
#14.6 - Moved EntityModerationRouteProvider to \Drupal\content_moderation\Entity.
Comment #17
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedGreat clean up, thoughts below.
Maybe we should just copy these cache clears into both functions?
Maybe these doc comments can be
Implements hook_something().
like the hooks themselves? Do we have any precedent for this?I think we need to move ContainerFactoryPluginInterface back to being directly used by our implementation. We can't say for sure all implementations will need to use a factory and
::create
is an implementation detail that shouldn't be part of the public api.Maybe this is the perfect spot to explain why this exists if we don't decide to make it a new method.
Comment #18
timmillwood#17.1 - yeh, that works too.
#17.2 - I made it universal to use
@see
, we don't have anywhere else in core doing this, but as@see
is a recognised format it seemed to make sense.##1.2a - Fixed
#17.3 - Added
@param $entity
.Comment #19
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented+1 RTBC. Hopefully @tim.plunkett can review.
Comment #20
tim.plunkettThat's a good change, thanks @Sam152
And thanks to @timmillwood for the quick turnaround.
Comment #21
catchThis looks like good clean-up.
I already did a major architectural review of content_moderation when it was first slated for commit (which ended up with the refactoring of workflows and some other patches), so haven't done a massive re-review - things are in a good spot generally.
Committed/pushed to 8.5.x and 8.4.x, thanks!
Comment #22
tacituseu CreditAttribution: tacituseu commentedPush didn't make it.
Comment #23
tacituseu CreditAttribution: tacituseu commentedComment #24
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOne of two things that bugs me a lot with Content Moderation is the revision_tracker table, so I posted a patch in #2865579: Rewrite the 'Latest revision' views filter and remove the revision_tracker table hoping that we can get rid of it.
The other one is that
\Drupal\content_moderation\ModerationInformationInterface::getWorkflowForEntity()
hard-codes the assumption that an entity bundle can have only one Content Moderation workflow, but I don't think we can do anything about that anymore..Comment #26
catchI've RTBCd the revision_tracker table removal.
Also actually pushed the commits here now.
Comment #29
catchLots of test fails so rolled this back.
Comment #31
tacituseu CreditAttribution: tacituseu commented@catch: Patch is ok, but instead of it only CS fix removing unneeded use statement was uploaded (e.g. check c7c9a329)
Comment #32
timmillwoodGood spot @tacituseu, I was just about to say, I can't replicate this issue locally.
Comment #33
catchOuch! Sorry about that and good spot. Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!