Problem/Motivation

In Workbench Moderation all hook implementations were abstracted out into services. When moving to core as Content Moderation we decided that as these were not for reuse, overriding, or extending they should be just plain classes. However now looking at them again plain classes are not the most friendly approach, and if we're looking for something to

Proposed resolution

Use ClassResolver.

For example:

/**
 * Implements hook_entity_base_field_info().
 */
function content_moderation_entity_base_field_info(EntityTypeInterface $entity_type) {
  return   \Drupal::service('class_resolver')
    ->getInstanceFromDefinition(EntityTypeInfo::class)
    ->entityBaseFieldInfo($entity_type);
}

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#18 use_class_resolver_for-2781207-18.patch10.23 KBtimmillwood
#16 interdiff.txt644 bytestimmillwood
#16 use_class_resolver_for-2781207-16.patch10.22 KBtimmillwood
#13 use_class_resolver_for-2781207-13.patch10.19 KBtimmillwood
#9 interdiff.txt5.73 KBtimmillwood
#9 use_class_resolver_for-2781207-9.patch10.18 KBtimmillwood
#4 use_class_resolver_for-2781207-4.patch10.65 KBtimmillwood
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

timmillwood created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

So do you propose to use the ContainerInjectionInterface as we have it for controllers?

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

@dawehner - I think that would be a good thing, yes.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Assigned: timmillwood » Unassigned
Status: Active » Needs review
FileSize
use_class_resolver_for-2781207-4.patch10.65 KB

Patch updates ContentPreprocess, EntityOperations, and EntityTypeInfo to implement ContainerInjectionInterface. The Create method is added inline with the interface spec. Then _content_moderation_class_resolver is added to content_moderation.module to resolve the class and statically cache it.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented 
+++ b/core/modules/content_moderation/content_moderation.module
@@ -37,79 +38,89 @@ function content_moderation_help($route_name, RouteMatchInterface $route_match)
+  static $instances = [];
+
+  $parts = [
+    'service_container',
+    \Drupal::getContainer()->getParameter('kernel.environment'),
+    \Drupal::VERSION,
+    Settings::get('deployment_identifier'),
+    PHP_OS,
+    serialize(Settings::get('container_yamls'))
+  ];
+  $container_cache_key = implode(':', $parts);

Instead of doing it here we could introduce a ClassResolverWithState class which does the same and ensures that there is just one instance of it, by having this special class resolver in the container.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

@dawehner - briefly discussed this with Alex on IRC

alexpott
timmillwood: in a generic method in the content_moderation.module
timmillwood: and then a followup issue to discuss generalising for core

There's a number of ways to do it, I was thinking it could just be a different method within \Drupal\Core\DependencyInjection\ClassResolver, or it could equally be an update of the current method.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha at Acquia for Acquia commented
Component: content_translation.module » content_moderation.module

Fixing component

Status: Needs review » Needs work

The last submitted patch, 4: use_class_resolver_for-2781207-4.patch, failed testing.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
use_class_resolver_for-2781207-9.patch10.18 KB
interdiff.txt5.73 KB

Removing the static cache.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

This looks basically how I expect it to look like. Thank you @timmillwood
Its a clear scope of improvements.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: use_class_resolver_for-2781207-9.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Issue tags: +Needs reroll, +Drupalaton, +Novice

.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs work » Fixed
Issue tags: -Needs reroll, -Novice
FileSize
use_class_resolver_for-2781207-13.patch10.19 KB

Simple re-roll.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Fixed » Needs review

It's not fixed, it needs review!

Status: Needs review » Needs work

The last submitted patch, 13: use_class_resolver_for-2781207-13.patch, failed testing.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
use_class_resolver_for-2781207-16.patch10.22 KB
interdiff.txt644 bytes

Fixing issue.

Status: Needs review » Needs work

The last submitted patch, 16: use_class_resolver_for-2781207-16.patch, failed testing.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
use_class_resolver_for-2781207-18.patch10.23 KB

Re-rolling

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Looks great for me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed fe33c4b to 8.3.x and 0f7d56e to 8.2.x. Thanks!

I think this pattern gets us the best of both worlds. A module's hook implementations are not public services (and this is good because they are not API) and the dependencies are encapsulated in the class.

diff --git a/core/modules/content_moderation/content_moderation.module b/core/modules/content_moderation/content_moderation.module
index be7dcc3..1b30cc4 100644
--- a/core/modules/content_moderation/content_moderation.module
+++ b/core/modules/content_moderation/content_moderation.module
@@ -18,7 +18,6 @@
 use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Routing\RouteMatchInterface;
 use Drupal\Core\Session\AccountInterface;
-use Drupal\Core\Site\Settings;
 use Drupal\node\NodeInterface;
 use Drupal\node\Plugin\Action\PublishNode;
 use Drupal\node\Plugin\Action\UnpublishNode;

Removing unused use on commit.

  • alexpott committed fe33c4b on 8.3.x 
    Issue #2781207 by timmillwood, dawehner: Use class resolver for...

  • alexpott committed 0f7d56e on 8.2.x 
    Issue #2781207 by timmillwood, dawehner: Use class resolver for...

Status: Fixed » Closed (fixed)

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