Permissions: moderation states with the same name is unclear for user to assign to a role

I wonder if for both this issue, and over in #2738903: Multiple moderation states with the same label are confusing in the transition UI if we should rather than always add the machine name, first check to see if there are any duplicatly named transition states? That way, simple sites would continue to get the UX they have now without the machine name, and more complex sites would get the clarifying machine name.

This is looking really good.

One nitpick:

+++ b/src/Permissions.php
@@ -48,2 +56,24 @@
+  private function getUniqueLabels($args) {

Protected functions are preferred to private in Drupal coding standards.

Also, we need to make sure this method will allow the work in #2738903: Multiple moderation states with the same label are confusing in the transition UI to use the same logic. If not, a more general utility method should be introduced I think.

+++ b/src/Form/ModerationStateTransitionForm.php
@@ -68,9 +70,12 @@ class ModerationStateTransitionForm extends EntityForm {
+    $states = ModerationState::loadMultiple();
+    $unique_labels = Permissions::getUniqueLabels($states);

Since this is being used for both permissions and moderation state forms, perhaps the new method (getUniqueLabels()) should be on the ModerationStateInterface interface (and thus ModerationState class) itself?

One last nitpick and I think this is ready!

+++ b/src/Entity/ModerationState.php
@@ -78,4 +78,20 @@ class ModerationState extends ConfigEntityBase implements ModerationStateInterfa
+    foreach (self::loadMultiple() as $arg) {

While this probably won't matter here, in general, rather than self::, static:: is preferred since then final control of which method is used is left to the implementing class (see Late static bindings for further detail).

This looks great. For simple sites, there will be no noticeable change to the UI, but for complex sites with multiple transitions that might share a label, this is a big usability enhancement.

+++ b/src/Entity/ModerationState.php
@@ -78,4 +78,20 @@ class ModerationState extends ConfigEntityBase implements ModerationStateInterfa
+  public static function getUniqueLabels() {

diff --git a/src/Entity/ModerationStateTransition.php b/src/Entity/ModerationStateTransition.php
index 75e098c..7a4dbfc 100644

+++ b/src/Entity/ModerationStateTransition.php
@@ -141,4 +141,23 @@ class ModerationStateTransition extends ConfigEntityBase implements ModerationSt
+  public static function getUniqueLabels() {

This is essentially the same code - could we have it in a trait instead?

Other than that, I reckon this is ready to go

This looks good to go!

Hi @pericxc, thanks for this enhancement where in machine_name is displayed if there were any duplicate state label names.
We'll need to display machine_name in 'Manage moderation' page. While any content type enables moderation, machine_name display on this page is very helpful. Please see the attached screen grab - missing machine_name in Manage moderation.png

Thank you

Following up from #2757349: WI: Deal with scalability issues in the UI, which mentioned the cluttering of the permissions page. This patch unfortunately makes it worse. :( Spun off #2766467: Permissions: Make the permission description the name and drop the description to cut down UI clutter to talk about fixing this, since the other changes look good.

That I don't know, since I'm not a maintainer of this module. I'm guessing though that one of them will see this and respond soon. :)

As pointed out in #25 I think this 'fix it where we see it' approach is bound to miss some places.

Going to try a different approach.

This is the approach.

So the default label() method will always return the unique label.

No need to override it elsewhere.

Then if we need a non-unique label (eg in places where showing the original value in the form or when we're showing the nice names in the content edit form and machine names are inappropriate) we pass FALSE argument.

That way every other place the label is shown that there are duplicates, we don't need special code.

We can control when we are calling label(), but we can't control every other place in core and contrib etc. So we flip it.

This way, the default behaviour is enough.

Screenshots show no difference to previous patch.

Thoughts?

Also @pericxc to answer your question, I'm doing reviews of RTBC issues in modules I maintain every Friday with view to commit. This one was on today's list but I think we're not quite there yet.

If I were writing this:

  public static function getNonUniqueLabels() {
    $labels = [];

    foreach (static::loadMultiple() as $arg) {
      $label = $arg->label(FALSE);
      $labels[$label] = isset($labels[$label]);
    }
    
    return array_filter($labels);
  }

I think this has addressed all the outstanding points above. The code going in here can be refactored as needed for #2766467: Permissions: Make the permission description the name and drop the description to cut down UI clutter without too much effort I think.

I've manually tested this as well with multiple states with the same label.

Thanks, will commit on Friday all going to plan

Why don't we enforce unique labels?

Why don't we enforce unique labels?

Sites with many different workflows can have the same label (eg, Draft) for multiple states since these are site wide, and a state transition for one entity type might have completely different permissions than a state for another entity type.

I think core is planning on going a different way here. Having global states and transitions doesn't really make make sense. We're thinking of taking a similar approach to workflow. See #2779647: Add a workflow component, ui module, and implement it in content moderation

Adding related core content moderation issue.