Convert static functions in Content moderation state Entity class to Service for easier Unit testing

Sorry didn't mean for this to fall of my radar.

Removing the needs tests tag as I can see they were added

Appears there are failures that need to be fixed.

Thanks! Will keep an eye out in my email for this to come back around.

I've fixed the tests, @smustgrave - please take a look!

Pinged @Sam152 as the content moderation subsystem maintainer.

Left a few comments for typehints.
Tagged for a change record for the new service.

Everything else looks good

Thanks!

Hi @Murz

I've also left a few comments. Take a look at them if you don't mind. Nice job man.

Regards,
Andrei

@andrei.vesterli, @smustgrave I've made fixes by all remarks, please take a look!

I have a question about how the types should be exact in the function:

  public function loadFromModeratedEntity(EntityInterface&RevisionableInterface $entity): ?ContentModerationStateInterface {

While Intersection is still not supported by phpcs (https://github.com/squizlabs/PHP_CodeSniffer/issues/2410) - is it ok to leave this as Union like EntityInterface&RevisionableInterface $entity, or it's better to keep just the EntityInterface $entity there?

The RevisionableInterface interface is required for this line:

        ->condition('content_entity_revision_id', $entity->getLoadedRevisionId())

Still needs a change record I believe.

I've created a change record here https://www.drupal.org/node/3341126 → - please take a look. Do I need to add or rework something there?

Nope that CR looks good to me. Going to ping Sam152 for subsystem review

Personally I would avoid adding deprecations, disrupting existing code and introducing a new public service in place of APIs marked @internal, to satisfy testing requirements outside of core. Custom code always has the option of adding a new interface that thinly wrap this functionality.

More subjectively I don't think the example given is a great candidate for a unit either, the body of the function itself doesn't contain a lot of complex branches. There is a higher risk that the configuration may change in a some unexpected way, the ContentModerationState entity being MIA or the entity being passed in not being under moderation for whatever reason etc.

  protected function getModerationStatusLabel(EntityInterface $entity) {
    return \Drupal\content_moderation\Entity\ContentModerationState::loadFromModeratedEntity($entity)
      ->workflow
      ->entity
      ->get('type_settings')['states'][$entity->moderation_state->value]['label'];
  }

Don't take it personally though, in my experience Drupal projects are generally not unit testable. Most failure and risk comes from bootstrapping complex structures and behaviours out of the database, plugging-in and alter-ing as a design pattern, on top of a core product and suite of modules that are evolving underneath you. You sort of have to boot the whole thing up before you can be confident anything is actually working :)

Removing the tag. I'm more than fine with not having the final say on this one, if someone else feels strongly about pushing it forward.

On the next self-review I've found the way to simplify the things without creating a new service, so I've moved the loadFromModeratedEntity() to the content_moderation.moderation_information service.
Also get rid of deprecated function call in all related unit tests.

So this is a complex one that I can't fully speak to. From the changes I've seen I give it +1 for RTBC will see if someone else will review.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The original function in the Core was changed by #2860097: Ensure that content translations can be moderated independently → , so I've squashed commits and rebased on the fresh Core branch.

Will send to committers for their thoughts.

Added some feedback to the MR.

Corrected everything according to the comments, please review.

Small text change.

Thank you for the review, committed fixes for all remarks, please review.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased on fresh 11.x

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased again to fix CI.

Made a very minor tweak changing 10.1.0 to 10.2.0, don't think that should exclude me from marking.

Checking 2717 though and appears all feedback from previous reviews have been addressed.

There are some comments on the MR to be resolved.