Add a views sort handler for sorting content by moderation state

Assuming this is the issue. There is no table sorter for headers.

Workaround: Add a "Content moderation state" relationship to your view. Add a "Moderation state" field that uses the relationship. Enable the field overwrite functionality and use the output for the original moderation state field. Hide the original moderation state field.

This will give you a sortable moderation state column that uses the correct display label.

Finally got around to addressing this. This patch:

• Adds support for sorting on the field (exposed or otherwise).
• Click sorting via the table display type.
• Tests for both these types of sorting, with normal base table and the revision base table.

Fixing docblock.

The patch looks great to me!

+++ b/core/modules/content_moderation/src/Plugin/views/field/ModerationStateField.php
@@ -0,0 +1,48 @@
+    $this->entityTypeManager = $entity_manager;

I was a bit unsure about this part, but we're only using the entityTypeManager property internally in ModerationStateJoinViewsHandlerTrait, so maybe it's ok to do it like this.

+++ b/core/modules/content_moderation/src/Plugin/views/field/ModerationStateField.php
@@ -0,0 +1,48 @@
+    $this->entityTypeManager = $entity_manager;

This property is declared dynamically so it'll be public. We need to define it on this class - or somehow fix EntityField to use the specific entity manager services - which feels unlikely in this issue.

Thanks for the review, good catch.

Just dropping by here out of curiosity, and because I think its a good idea :)

Patch is looking great, my only concern is this change:

+++ b/core/modules/content_moderation/src/ViewsData.php
@@ -79,11 +79,12 @@ public function getViewsData() {
-          'id' => 'field',
+          'id' => 'moderation_state_field',

@@ -103,11 +104,12 @@ public function getViewsData() {
-          'id' => 'field',
+          'id' => 'moderation_state_field',

Does this mean we need an upgrade path for existing views?

Yes!

Tagging.

+++ b/core/modules/content_moderation/src/Plugin/views/field/ModerationStateField.php
@@ -0,0 +1,55 @@
+   * The entity manager service.
+   *
+   * @var \Drupal\Core\Entity\EntityManagerInterface
+   */
+  protected $entityTypeManager;

The mismatch here makes me want to have the proper entity type service even more :/

Yeah, I agree. Lets inject the proper service here in addition to looking into #11.

Adding the upgrade path and tests for that.

+++ b/core/modules/content_moderation/tests/fixtures/update/drupal-8.4.0-content_moderation_installed.php
@@ -31,6 +31,11 @@
+  ->values(array(
+    'collection' => '',
+    'name' => 'views.view.moderated_content',
...
+  ))

Realising why this view wasn't in the fixture originally: it was introduced in 8.5. To ensure the integrity of the 8.4 fixture, we should add this is another fixture.

Updating.

Addressing #14.

The latest patch looks very good, I spotted just a couple of minor things:

1. +++ b/core/modules/content_moderation/src/Plugin/views/field/ModerationStateField.php
   @@ -0,0 +1,74 @@
   +    // This could be derived from the content_moderation_state entity table
   +    // mapping, however this is an internal entity type whose storage should
   +    // remain constant.
   

   That's not necessarily true, for example the JSON entity storage could store the whole entity data in a single table column, so we should do to the right thing and get the column name from the table mapping :)

2. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationStateViewsFieldUpdateTest.php
   @@ -0,0 +1,41 @@
   + * @group legacy
   + * @see content_moderation_post_update_views_field_plugin_id
   

   Let's add an empty line between these, and append () to the post update function name :)

Good catch, thanks for the review.

Addressing feedback from #21.

This looks ready to me.

+1, looks good to me as well.

Thanks @Sam152 - upgrade path & tests look good to me, RTBC++

@jibran pinged me and made me aware of #2810485: Allow more maintainers to grant credit. He answered some questions for me in slack when I was writing the upgrade path, so granting request for credit here. Adding @amateescu, @Manuel Garcia and @alexpott for reviews while I'm at it.

error: patch failed: core/modules/content_moderation/content_moderation.post_update.php:5
error: core/modules/content_moderation/content_moderation.post_update.php: patch does not apply

No longer applies :(

Rerolling

Another reroll, this time with less syntax errors guaranteed!

Adding a secondary sort criteria of the langcode. I think the two different language revisions were sorting inconsistently locally and on the bot when they had the same state name.

Hm, it's possible views doesn't support secondary sort criteria mixed with exposed input. This will possibly make testing this in a flexible way tricky.

After digging into this, I believe this is the only way to maintain the same level of test coverage with consistent builds.

This was already RTBC before a reroll and the interdiff in #37 in case anyone is interested in reviewing the changes since then.

Hi @Sam512,

I applied the patch in #37 and now I am able to sort the content using the state but when I did the drush updb, I got this error -

[error]  Class "ConfigEntityUpdater" does not exist.
 [error]  Update failed: content_moderation_post_update_views_field_plugin_id
 [error]  Update aborted by: content_moderation_post_update_views_field_plugin_id

Please test this on 8.7.x HEAD if you are reviewing the issue. This patch hasn't been backported to previous versions, so if you're running 8.6 or 8.5, you'll manually have to backport this if you are evaluating it.

It's likely in your case the ConfigEntityUpdater wasn't use'd at the top of the file, since it was probably included by another update hook that hasn't made it into the release you are running.

Ahh! yes I was using that patch on 8.6. Thanks, adding use statement made it work. I believe that was the only missing thing for 8.6 compatibility, if not please tell me I can work on patch for 8.6 backport.

FWIW, RTBC++ on #37

I reviewed #2862041: Provide useful Views filters for Content Moderation State fields for cacheability, so I figured I'd do the same here.

1. +++ b/core/modules/content_moderation/src/Plugin/views/field/ModerationStateField.php
   @@ -0,0 +1,75 @@
   +class ModerationStateField extends EntityField {
   

   👍 This inherits \Drupal\Core\Plugin\ContextAwarePluginBase::getCacheTags(), which inspects the used contexts to determine the cache tags.

   EntityField is known to handle cache tags correctly, which means this should too.

2. +++ b/core/modules/content_moderation/tests/src/Kernel/ViewsModerationStateSortTest.php
   @@ -0,0 +1,187 @@
   +    // Ascending order will see 'published' followed by 'zz_draft'.
   +    $this->assertSortResults('test_content_moderation_state_sort_base_table', 'nid', 'ASC', [
   +      ['nid' => $second_node->id()],
   +      ['nid' => $first_node->id()],
   +    ]);
   +    // Descending will reverse the order.
   +    $this->assertSortResults('test_content_moderation_state_sort_base_table', 'nid', 'DESC', [
   +      ['nid' => $first_node->id()],
   +      ['nid' => $second_node->id()],
   +    ]);
   

   🤔 It would be nice if this were to verify that modifying the moderation state of either node would invalidate the cached response. (And that both previous requests resulted in cacheable responses.)

   Since this is only ever going to be used for admin views, I'm not sure how valuable this is though. I'll leave that to core committers to decide.

This needed a reroll because we already introduced the drupal-8.5.0-content_moderation_installed fixture in #2983958: Users expect the "latest translation affected revision" not the "latest revision" in the views UI when moderating content.

1. Nice, clicking through to these, they don't really have an intersection with the kind of customisations we're making in the subclass, so I think we're covered.
2. I don't really know if it should be the concern or if it would even be possible to test the cacheability of these handlers, if they have very plain cacheability requirements (that is, changes are only ever correlated with changes in the entity field data) and don't leverage any tags that aren't already covered by the "{$entity_type}_list" tag.

   My line of thinking is: how would you ever isolate that the invalidation came from the sort plugin vs the tags that are already present from the other views subsystems (such as the display plugins) that ensure the "{$entity_type}_list" tag is already part of the built output of the page.

   In short, I don't think you could ever produce a failing test by overriding this particular plugin and returning NULL for all the cacheability methods.

   Not sure if I'm missing anything though.

1. Indeed!
2. Very good point. I think you're right. That, combined with the fact that this will likely only ever be used for admin views, is sufficiently convincing IMHO. I already didn't feel strongly about this point anyway, hopefully I made that clear :)

Thanks for following up, all sounds great to me :)

This just needed a reroll against #2915383: The moderation_state base field is added to all revisionable entity types even if they do not have moderation enabled.

This was only rerolled and it looks like #45 is a sign-off from @Wim? :)

RTBC +1

Queued this on a bunch of different DBs, looking solid. The sqlite fail was after it had reached the nightwatch tests: Error retrieving a new session from the selenium server.

1. +++ b/core/modules/content_moderation/src/Plugin/views/field/ModerationStateField.php
   @@ -0,0 +1,75 @@
   +    if (!isset($this->aliases[$column_name])) {
   +      $this->aliases[$column_name] = $this->tableAlias . '.' . $column_name;
   +    }
   

   🔧 I would have actually dropped the if condition. It is potentially unlikely that the aliases is set already, but even if it is, we don't really save anything by adding this condition. It rather opens up the question, why it might or might not be set yet.

2. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -21,6 +22,8 @@
     */
    class ModerationStateFilter extends InOperator implements DependentWithRemovalPluginInterface {
    
   +  use ModerationStateJoinViewsHandlerTrait;
   +
      /**
       * {@inheritdoc}
       */
   @@ -109,45 +112,6 @@ public function getValueOptions() {
   
   @@ -109,45 +112,6 @@ public function getValueOptions() {
        return $this->valueOptions;
      }
    
   -  /**
   -   * {@inheritdoc}
   

   Nice extraction of logic.

3. +++ b/core/modules/content_moderation/tests/modules/content_moderation_test_views/content_moderation_test_views.module
   @@ -0,0 +1,22 @@
   +
   +/**
   + * Implements hook_views_query_alter().
   + *
   + * @see \Drupal\Tests\content_moderation\Kernel\ViewsModerationStateSortTest::testSortRevisionBaseTable()
   + */
   +function content_moderation_test_views_views_query_alter(ViewExecutable $view, QueryPluginBase $query) {
   +  // Add a secondary sort order to ensure consistent builds when testing click
   +  // and table sorting.
   +  if ($view->id() === 'test_content_moderation_state_sort_revision_table') {
   +    $query->addOrderBy('node_field_revision', 'vid', 'ASC');
   +  }
   +}
   

   I would have rather expected that the view itself has an additional sort configured, instead of altering the query, given that the sort might or might not be added at the front/back.
   Adding the hook_views_query_alter() seems to make it much harder to understand.

Hey @dawehner, thanks for the review.

1. I'm fine with that change. Lets see what the bot says.
2. :)
3. I tried this in #34, but these secondary sorts are ignored when click sorting or accepting exposed input, both of which need to be tested because click sorting is integrated via the field plugin, not the sort plugin.

Whoops, missed the interdiff somehow, but the only change made was addressing point 1.

Patch is looking great to me, awesome work - RTBC++
Did a minor cleanup related to CS / docblocks while I was looking at it.

Here's a backport of #56 to 8.6.x

Reuploading #56 so it's clear which patch needs review.

Failing another follow-up review, RTBC for the interdiff in #56 + the scope of changes made as a result of #52 was the feedback in #52.1, so very minor.

After applying the patch via composer after running drush updb I get the following error:

[notice] Update started: content_moderation_post_update_views_field_plugin_id
[error] Class "ConfigEntityUpdater" does not exist.
[error] Update failed: content_moderation_post_update_views_field_plugin_id
[error] Update aborted by: content_moderation_post_update_views_field_plugin_id
[error] Finished performing updates.

I suggest adding use Drupal\Core\Config\Entity\ConfigEntityUpdater to the top of content_moderation.post_update.php.

Re #63 - that is already in the file since 8.7.x (see https://cgit.drupalcode.org/drupal/tree/core/modules/content_moderation/...) - it's also still there on 8.8.x.

I believe the tests were failing due to #3023981: Add @trigger_error() to deprecated EntityManager->EntityRepository methods, which means we can simplify ModerationStateField, so yay.

Great clean up, thanks for making this green again.

We have a deprecation test that is failing. Keeping up with HEAD and fixing coding standards.

Adding review credits

Committed 81d8e2a and pushed to 8.8.x. Thanks!

Not back-porting immediately because I think the update hook is disruptive at this stage of the 8.7 cycle.

Yay!

So is this fixed then or is a backport a possibility?

@larowlan, @catch, and I discussed this and agreed that it's more of a missing feature than a bug per se. It's a smidge disruptive and includes lots of additions that are only typically allowed in minor releases. So, setting fixed against 8.8.x.

Thanks everyone!

Fair enough, thanks for the update @xjm!