Provide useful Views filters for Content Moderation State fields

This was defined as MUST-HAVE by @webchik in #2755073-10: WI: Content Moderation module roadmap:

So it seems like another must-have (but not commit-blocking) is to get that widget to be a drop-down instead, since I don't think we want this to be the content author experience.

Working on this.

Is this a duplicate of #2859384: Provide checklist of workflow states in Views filter?

Yes, it seems so. I closed that since this is linked in the meta issues, I will summarize any relevant info here.

I spoke about this briefly in IRC, but I think it would be great to provide good views support for the computed content moderation state field on the entities under moderation, without the need to add a relationship to the ContentModerationState entity.

If we can do that here for filtering, #2852067: Add support for rendering computed fields to the "field" views field handler solves being able to view the state.

After we have filtering and support for viewing the computed field on the moderated entity, I think we should remove the views relationship to the ContentModerationState entirely, honoring it's @internal status by putting it out of sight of users.

Opened #2894479: Deprecate the Views relationship from moderated content to the "content_moderation_state" entity to keep this issue focused.

I'm not going to get to this one before Monday morning.

Please see attached patch for an attempt to add this feature

Please integrate this "feature" in the next drupal core 8.3.x update. 8.4.x is far away isn't? It's a really basic and required funcionality.

Going to have a look at this. As per #6 going to try and ensure the filter is applied to the computed field on the moderated entity, not the ContentModerationState entity, to ensure no relationship is necessary in the views configuration.

Patch attached allows filtering on the computed field, directly on the entity being moderated, no relationships required.

Having a look at this, however the IS needs some love :)

@plach pointed out this doesn't seem to work with a view of node revisions. NW for fixing that + tests.

I wasn't able to reproduce the bug, but adding a test case.

The field map is required in the assert to make this fail properly.

Code is looking great and on manual testing things look very nice too, awesome work!

I was able to find a problem with content revision views and multilingual: apparently this view works perfectly on a monolingual site but adding a language breaks the query.

Aside from that, I have only a few very minor remarks:

1. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,92 @@
   + * Filter based on the moderation state of an entity.
   

   We should use a third person verb to start the doc block, what about "Provides a filter..."?

2. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,92 @@
   +   * Create an instance of ModerationStateFilter.
   

   Creates

3. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,92 @@
   +    $this->query->addRelationship('cms', $join, 'content_moderation_state_field_revision');
   

   Views normally uses more verbose aliases, what about replacing cms with content_moderation_state for consistency?

4. +++ b/core/modules/content_moderation/tests/src/Kernel/ViewsModerationStateFilterTest.php
   @@ -0,0 +1,173 @@
   +   * Test the list of states in the filter plugin.
   

   Tests

5. +++ b/core/modules/content_moderation/tests/src/Kernel/ViewsModerationStateFilterTest.php
   @@ -0,0 +1,173 @@
   +    // Adding nodes to the editorial workflow will enable all of the editorial
   +    // states.
   

   "nodes" is a bit misleading, what about "a content type"?

6. +++ b/core/modules/content_moderation/tests/src/Kernel/ViewsModerationStateFilterTest.php
   @@ -0,0 +1,173 @@
   +   * @param array $states
   +   *   The states which should appear in the filter.
   ...
   +   * @param array $filters
   +   *   An array of filters to apply to the view.
   

   Shouldn't these be string[]?

7. +++ b/core/modules/content_moderation/tests/src/Kernel/ViewsModerationStateFilterTest.php
   @@ -0,0 +1,173 @@
   +   * @param array $nodes
   +   *   Nodes to assert are in the views result.
   

   Shouldn't this be \Drupal\node\NodeInterface[]?

Thanks for the review, will address the feedback. This shows us the failure, but I suspect we'll also need to add another part of the 'extra' section to ensure langcode becomes part of the join criteria, unless that's auto-magically handled.

FWIW, the fail boils down to:

Integrity constraint violation: 1052 Column 'langcode' in field list is ambiguous

Can't see any obvious fixes for it yet.

I'll try to have a look to that failure during the weekend if you don't beat me to it, tomorrow I'll be AFK.

I've opened a follow up for what I believe to be the cause #2896381: "TranslationLanguageRenderer" uses the wrong table for the "langcode" column with entity revision views.

This patch addresses #18 and adds multilingual support + tests.

It will go green as soon as the blocker is resolved.

#20 already has an issue

Thanks, @tacituseu, I've created a test and patch for the blocker. @plach, if have time to look at the progress on it, that'd be great. Looks like you're already familiar with the issue.

Postponed on #2446681: Error "Column 'langcode' in field list is ambiguous" thrown due to TranslationLanguageRenderer not rendering a field from a relationship.

Actually postponed on #2896381: "TranslationLanguageRenderer" uses the wrong table for the "langcode" column with entity revision views.

The test coverage looks great!

1. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,98 @@
   +  /**
   +   * Creates an instance of ModerationStateFilter.
   +   */
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager) {
   ...
   +    $this->entityTypeManager = $entity_type_manager;
   

   Nitpick: let's document the parameters and provide class documentation for them

2. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,98 @@
   +      if (in_array($this->getEntityType(), $workflow->getTypePlugin()->getEntityTypes())) {
   

   Let's use string comparison, you never know :)

3. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,98 @@
   +      'table' => 'content_moderation_state_field_revision',
   +      'field' => 'content_entity_revision_id',
   

   This information could be fetched from the content moderation state entity key definition, right?

4. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,98 @@
   +        'field' => 'langcode',
   

   Is there a reason you couldn't get the entity key in the content_moderation_state entity?

Here's a patch combined with #2896381-9: "TranslationLanguageRenderer" uses the wrong table for the "langcode" column with entity revision views to check whether we are done here. This includes a one-line fix to a test module to fix a couple failures.

Setting to NR for the bot but this is still postponed.

Missed #30, setting to NW then.

Looks like your work in the blocker is going to work for us here. I've +1d the approach in that issue, but having written the test, if it would be great if @dawehner could RTBC.

#30 is good feedback, happy to look at it. Re: #30.3 and #30.4, do we usually get that information from entity definitions when we are working with only a single specific entity type? If those fields ever changed, the tests would inform us right?

Re: #30.3 and #30.4, do we usually get that information from entity definitions when we are working with only a single specific entity type? If those fields ever changed, the tests would inform us right?

If they were ever changed in core, yes, but contrib and custom code are free to rename things to whatever they want, so we should always get the table and field names from the entity definition.

Addressing the feedback in #30. Not combined with the fix, so will go read.

Looks RTBC to me, I'll move this back to postponed as soon as the bot is done testing.

Perfect, confirming RTBC by my manual tests. As soon as the testbot tests succeed this should become part of the module. It's really essential for building useful moderation views.

Thank you all so much.

@Sam152 thanks! patch #36 works great!

I think it needs a reroll for 8.5 (one FAILED)

patching file core/modules/content_moderation/config/schema/content_moderation.schema.yml
Hunk #1 succeeded at 16 with fuzz 2 (offset 10 lines).
patching file core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
patching file core/modules/content_moderation/src/ViewsData.php
Hunk #1 succeeded at 203 (offset 2 lines).
Hunk #2 succeeded at 223 (offset 2 lines).
patching file core/modules/content_moderation/tests/modules/content_moderation_test_views/config/install/views.view.test_content_moderation_state_filter.yml
patching file core/modules/content_moderation/tests/modules/content_moderation_test_views/config/install/views.view.test_content_moderation_state_filter_entity_test.yml
patching file core/modules/content_moderation/tests/modules/content_moderation_test_views/config/install/views.view.test_content_moderation_state_filter_revision_table.yml
patching file core/modules/content_moderation/tests/modules/content_moderation_test_views/content_moderation_test_views.info.yml
Hunk #1 FAILED at 8.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/content_moderation/tests/modules/content_moderation_test_views/content_moderation_test_views.info.yml.rej
patching file core/modules/content_moderation/tests/src/Kernel/ViewsModerationStateFilterTest.php
patching file core/modules/system/tests/modules/entity_test/src/Entity/EntityTestNoBundle.php

Just to be clear: I was talking of the patch #36
And, after applying it on 8.5, I got the hideous The handler for this item is broken or missing. The following details are available:

Rerolled for latest 8.4.x. This will still fail until the dependency gets in (#2896381: "TranslationLanguageRenderer" uses the wrong table for the "langcode" column with entity revision views).

Removing calls to removed methods.

Many thanks for the reroll.
I have applied #49 on 8.5 and it apply more cleanly.
Here is the only problem

patching file core/modules/content_moderation/config/schema/content_moderation.schema.yml
patching file core/modules/content_moderation/tests/modules/content_moderation_test_views/content_moderation_test_views.info.yml
Hunk #1 FAILED at 8.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/content_moderation/tests/modules/content_moderation_test_views/content_moderation_test_views.info.yml.rej

Unfortunately, the problem remains (after cache clear): when I tried to add the moderation state field in the view UI, I got the following message:

The handler for this item is broken or missing.

Thanks @Sam152! Tested #49, it works great with 8.4.x.

Just some minor nits.

1. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,106 @@
   +      if (in_array($this->getEntityType(), $workflow->getTypePlugin()->getEntityTypes(), TRUE)) {
   +        $states += $workflow->getTypePlugin()->getStates();
   

   Let's store plugin in a local variable and not call the method twice.

2. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,106 @@
   +      'operator' => '=',
   

   This is not needed.

3. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,106 @@
   +    $join = Views::pluginManager('join')->createInstance('standard', $configuration);
   

   We have seen some nasty bugs related to join configurations and it's so easy to write the views data wrong for joins. Can we please have a \Drupal\Tests\views\Kernel\Plugin\JoinTest kind of tests for this, just to make sure that in future whenever someone changes this they have to update the test as well.

Can we please also have some screenshots of the filters and resultant queries?

@Sam152: At the beginning of this issue, I can read

2-Make it work with multilingual.

Is it because my site is multilingual that #49 doesn't solve the issue or is it because it is 8.5?

Issue summary just needs an update. The patch works with multilingual.

Re: #52.3, I don't know what this would look like, we don't want to retest the abstraction we're using?

I had a quick chat with @dawehner and he agreed with #52.3.

dawehner: do you think we need specific test for #52.3 in https://www.drupal.org/node/2862041#comment-12217439? If not then can you please reply to the issue?
jibran: I would love to see that

Like I said in #56, what does this actually look like? Can you add it to the patch?

Addressing #52.(1|2) and filing a follow-up.

@jibran, this patch does not add a new views join plugin, so I also don't understand #52.3. Can you open a followup and explain in more detail what you meant?

Otherwise, the patch looks really good!

I'm working on a patch. I'll post soon.

Added asserts for join config.
Here is resultant query:

SELECT node_field_data.langcode AS node_field_data_langcode, node_field_data.nid AS nid, node_field_revision_content_revision_tracker.vid AS node_field_revision_content_revision_tracker_vid
FROM 
{node_field_data} node_field_data
LEFT JOIN {content_revision_tracker} content_revision_tracker ON node_field_data.nid = content_revision_tracker.entity_id AND (content_revision_tracker.entity_type = :views_join_condition_0 AND content_revision_tracker.langcode = node_field_data.langcode)
INNER JOIN {node_field_revision} node_field_revision_content_revision_tracker ON content_revision_tracker.revision_id = node_field_revision_content_revision_tracker.vid AND (content_revision_tracker.entity_type = :views_join_condition_2 AND node_field_revision_content_revision_tracker.langcode = content_revision_tracker.langcode)
LEFT JOIN {content_moderation_state_field_revision} content_moderation_state ON node_field_revision_content_revision_tracker.vid = content_moderation_state.content_entity_revision_id AND (content_moderation_state.content_entity_type_id = :views_join_condition_4 AND content_moderation_state.langcode = node_field_revision_content_revision_tracker.langcode)
WHERE content_moderation_state.moderation_state IN (:db_condition_placeholder_6)

Still looks good apart from these two nitpicks, which could be fixed on commit :)

1. +++ b/core/modules/content_moderation/tests/src/Kernel/ViewsModerationStateFilterTest.php
   @@ -102,6 +102,7 @@ public function testStateFilterViewsRelationship() {
    
   +
        // The latest revision for both nodes is a draft status.
   

   Extra new line here.

2. +++ b/core/modules/content_moderation/tests/src/Kernel/ViewsModerationStateFilterTest.php
   @@ -227,10 +228,20 @@ protected function assertPluginStates($states) {
        $view->execute();
   +    $query = $view->getQuery();
   ...
   +    $this->assertEquals('langcode', $configuration['extra'][1]['left_field']);
        $expected_result = [];
   

   And here we could have some new lines to delimit these assertions.

Here we go.

Looks good. RTBC++

Adding screenshots to IS as requested.

Thanks, Sam.

I'll try and go back to find it, but the original must-have was that the Content view (or, failing that, a sub-tab) have a moderation state drop-down when Content Moderation module (or Workflow or whatever makes most sense) is installed. Because without that, people within the content approval flow get hard-blocked and can't actually "do" anything, because there's no way (without engaging the services of a Views Master™) to get a list of content in a Draft state:

The preview screenshot at https://www.drupal.org/files/issues/Screen%20Shot%202017-08-14%20at%2012... looks great; we just need to either apply that to the default Content view, or if that's too hard, add a "Moderate" or something sub-tab to that view that exposes the filter.

We can go over this on tomorrow's UX call if you'd like.

That would be great, but does it have to block this issue? This patch introduces the technical capability to add the filter. The details of integrating is mostly a UX conversation, whereas this has been a technical one so far.

We can do it as a follow-up, sure. But either way, it's a must-have and stands in the way of the module being marked stable, so whatever's easiest for you all.

Bumping back to needs review to reflect this.

I'm inclined to go for a follow-up, just based on narrower scoped issues being easier to review and commit.

Ok cool, moving this one back to RTBC, will create a new WI critical in a sec w/ the contents of #68.

Here we go: #2902187: Provide a way for users to moderate content

Based on the comment you made in #2755073: WI: Content Moderation module roadmap, I should clarify. This issue doesn't add a view or change any existing views configuration. It adds the ability for a site builder to add a views filter based on the "moderation state" field, which wasn't possible previously.

1. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,134 @@
   +  protected function opEmpty() {
   ...
   +  protected function opSimple() {
   

   Missing DocBlocks.

2. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,134 @@
   +    $this->ensureMyTable();
   ...
   +    $this->query->addWhere($this->options['group'], 'content_moderation_state.moderation_state', NULL, $operator);
   

   I'm not sure how ensureMyTable() is useful in here if we don't use $this->tableAlias in the addWhere().
   It's a shame we are overriding parent method only because 'content_moderation_state.moderation_state' is hardcoded.

3. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,134 @@
   +    if ($this->operator == 'empty') {
   

   This should probably be ===.

+++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
@@ -103,9 +103,11 @@ public function query() {
-    $this->ensureMyTable();

I think @gambry was actually suggesting to use the table alias returned by ::ensureMyTable() in the where condition instead of hardcoding content_moderation_state. Unless it's assuming the wrong table obviously :)

Ok, then ignore me :)

Back to RTBC, actually

After discussing #2902187: Provide a way for users to moderate content with Tim earlier today, I don't think we need to explore the COALESCE option. With the tweaks suggested at #2902187-54: Provide a way for users to moderate content over there, the view makes sense to do what it's currently doing, and only showing things in a "needs moderation" state.

1. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,139 @@
   +        $states += $workflow_type->getStates();
   

   If this is a non-associative array, the += will fail. Could result in a pretty nasty bug?
   Should use NestedArray::mergeDeep().
   IMO this needs test coverage.

2. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,139 @@
   +    $this->valueOptions = array_map(function($state) {
   

   array_map(function (StateInterface $state) { (missing space and typehint)

3. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,139 @@
   +   * Add the where query for NULL and NOT NULL operators.
   ...
   +   * Add the where query for "in" and "not in" operators.
   

   {@inheritdoc}

4. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,139 @@
   +    // @todo calculate dependencies in https://www.drupal.org/node/2901690.
   

   That is quite the punt, but okay

@tim.plunkett, thanks for reviewing! :)

Re #89:

1. \Drupal\workflows\WorkflowTypeInterface::getStates() always returns an associative array, I expanded its documentation because it wasn't clear in this regard. And that code is pretty well tested in \Drupal\Tests\content_moderation\Kernel\ViewsModerationStateFilterTest::testStateFilterStatesList()
2. Fixed.
3. Fixed as well.
4. :/

+++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
@@ -0,0 +1,140 @@
+  public function calculateDependencies() {
...
+    return parent::calculateDependencies();
+  }

Hmmm I don't think we should scope that to a followup. Wrong dependency info == data integrity issues.

We are also missing dedicated test coverage for the new view. There should be test coverage for all the twists and turns this issue has gone down in terms of what to display. This is not the kind of test coverage that should be scoped to followps.

Also let's not mark this issue RTBC with a combined patch with others that haven't landed yet, and please provide the no-test patches so it's easier to read what's part of this issue. Thanks!

I think #91 was meant to be on #2902187: Provide a way for users to moderate content?

In this instance, when states disappear because of a deleted workflow, views handles this gracefully and the states simply disappear from the list. The impact is some stale state keys in configuration until the filter is resaved. Is there any chance of it being split up like this to keep things moving? Given this is already kinda big, I thought a follow-up would make more sense.

+++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
@@ -0,0 +1,140 @@
+    $this->query->addRelationship('content_moderation_state', $join, 'content_moderation_state_field_revision');
...
+  protected function opEmpty() {
...
+    $this->query->addWhere($this->options['group'], 'content_moderation_state.moderation_state', NULL, $operator);
...
+  protected function opSimple() {
...
+    $this->query->addWhere($this->options['group'], 'content_moderation_state.moderation_state', $this->value, $this->operator);

Is there a reason we don't assign $this->tableAlias , given that this is what this relationship represents? Once done we could get rid of opEmpty and opSimple

+++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
@@ -0,0 +1,140 @@
+    // @todo calculate dependencies in https://www.drupal.org/node/2901690.

I'm with @tim.plunkett on this - that's a big gamble. I think without that, backporting this to 8.4 would be off the table. So does that reduce the urgency here?

@dawehner, that was asked before in #82 and replied to in #83 ;)

@amateescu
At least #82/#83 is not what I would have intended.

Note: I'm uploading the patch to see whether it fails ...

Updating the @todo with a comment as to why we shouldn't actually implement calculateDependencies.

@see #2901690: Implement ModerationStateFilter::calculateDependencies. for some further discussion on this.

Here is a fix for my experiment.

The changes from #96 and #101 seem to make sense, there's just one problem though: the patch that fixed "NULL" or "NOT NULL" handling in #77 did not add any tests for the fix, so we don't know if the latest changes from @dawehner have any impact on them..

Changes look good. Thanks @dawehner for jumping in.

+++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
@@ -74,6 +74,8 @@ public function getValueOptions() {
+      $node_table_alias = $this->query->ensureTable($this->table, $this->relationship);

@@ -84,7 +86,7 @@ public function ensureMyTable() {
+        'left_table' => $node_table_alias,

Let's change the name to $table_alias. Can be fixed on commit.

I think #103 is pretty serious though, we have no idea if "NULL" or "NOT NULL" handling is still working after the latest changes, since they are not tested.

I ran the tests in #108 against the filter in #64, the latest patch before the NULL handling was added. Fail patch attached should prove they work as described.

Fail expected, RTBC for #108.

Is there a reason why the views schema configuration lives in content_moderation.schema.yml instead of content_moderation.views.schema.yml?

I can't find any documentation/best practice about it but I can see all core modules have the views config schema definitions inside *.views.schema.yml .
If there isn't any and views schema should be in *.views.schema.yml for consistency, it can be done in a follow-up as there is already a pre-existing views.filter.latest_revision to be moved too.

1. +++ b/core/modules/content_moderation/config/schema/content_moderation.views.schema.yml
   @@ -0,0 +1,11 @@
   +  label: 'Moderation State Filter'
   

   Core UI labels should be sentence case rather than title case unless it's a proper noun (like the name of a module).

2. +++ b/core/modules/content_moderation/src/Plugin/views/filter/ModerationStateFilter.php
   @@ -0,0 +1,123 @@
   +    // Do not depend on the workflows which provide states that are utilised
   +    // in the filter. States cannot be depended on directly and can also be
   +    // common across multiple workflows. States and workflows with existing
   +    // content cannot be deleted, so the impact of not depending on them here is
   +    // mitigated.
   +    return parent::calculateDependencies();
   

   Hmmm. So I think this is saying that we won't have (e.g.) access bypasses due to a deleted state because no content would be in the state anymore if the filter were no longer functional.

   I'm still not entirely sure about this; "mitigating" the impact of potential data integrity issues is still not a reason to introduce them. We also get into trouble a lot when we make assumptions like this for core, but then contrib does something with the data that bypasses core expectations.

   At a minimum, we should have thorough functional test coverage for what we expect to happen in different scenarios where relevant workflows, states, provider modules, etc. are not available.

   I'd also want to see and review that the view is properly cleaned up (without noisy diffs in config), that it can be re-saved afterward, that it is still functional, etc.

For others, here's the discussion from the other issue. From @amateescu:

I don't think there's any reasonable way to implement this. The only way for a view to really depend on a workflow state is if we implement states as config entities.

Perhaps supporting all of these edge-cases is going a bit beyond what is necasary? The impact of a state disappearing when it's being used in a filter value seems to be fairly minimal, it simply no longer appears in the list. The only fallout as far as I can see is you have a state in your views config object which is stale until the filter is resaved.

I would go even further and say they are not even edge-cases, they are no-cases :) The view shouldn't care about states at all, they should only care about the workflow config entities that contain those states and their workflow type dependencies.

The reason is this: let's say we have a view with a moderation_state filter configured with a state, so it will show (or exclude) content with that state. Now, a workflow state can not be removed from a workflow when there is existing content with that state. In order to remove a state, the user also has to remove all the content using that state, at which point the view filter will have nothing to filter by because there's no more content with that state.

So your analysis above is correct, the impact of a state disappearing when it's being used in a filter value is zero :)

If we expand the reasoning for not depending on states, I think the outcome is the same: a workflow can only be deleted when there's no content using it, and, when that happens, the view doesn't show anything different based on the workflow existing or not. So.. yep, we don't need to depend on workflows at all.

And from @Sam152:

I think it could be argued that implementing something like this would mean the views filter configuration would not contain references to defunct states. Ie, a state with no content could be added to the filter, then deleted and the views filter configuration would still contain that state, even though it doesn't exist.

Given your analysis in #7 confirming what I suspected in #3, is this worth supporting? I would agree, no.

@xjm, please see #2901690-7: Implement ModerationStateFilter::calculateDependencies. and #2901690-9: Implement ModerationStateFilter::calculateDependencies. for the reasoning behind that code comment. I'll repeat a short version of it here as well:

Workflow states can not be deleted while there is existing content using them. When that content is gone and the state can be deleted, a views filter that excludes entities with that workflow state will have nothing left to exclude because there are no entities with the given state anymore.

There is no impact to mitigate because there is no impact at all. The code comment shouldn't say anything about mitigation actually :)

I think this part is still outstanding:

At a minimum, we should have thorough functional test coverage for what we expect to happen in different scenarios where relevant workflows, states, provider modules, etc. are not available.

I'd also want to see and review that the view is properly cleaned up (without noisy diffs in config), that it can be re-saved afterward, that it is still functional, etc.

But if we can agree in principle it'll be empty, maybe that isn't a blocker.

The "where provider modules are not available" part got me thinking that we should do this: #2904101: Do not allow modules that provide a workflow type plugin with existing data to be uninstalled

I think the testing in #123 is sufficient for to at least unblock this issue and further work on the follow-up view.

The primary concern that was blocking this issue, in my understanding was that a missing implementation of calculateDependencies would cause serious problems. If that has now been debunked, in my eyes it'd be best to get this in now with a follow-up (which I'll happily allocate time to) to cover more detailed testing that dives into:

• Functional testing around resaving the filter from the UI.
• The associated views configuration before/after resaving with deleted states.
• Proof the result-set is always unchanged based on @amateescu's analysis.

My reasoning for addressing this in a follow-up is to unblock folks working on #2902187: Provide a way for users to moderate content quickly. I'm hoping the above doesn't need to block that.

@xjm, if that approach is too much of the cart before the horse, it'd be great if you could chime in so we can prioritise work accordingly.

It would be cool to add to the filter settings the ability to select the workflow type. For example, I have 2 workflow types and both is used in different node types. In this case views filter will show all moderation states from both workflow types instead states of workflow type for current entity bundle.

Rerolling.

I think this only gets into 8.4.x is the follow-up view gets in, so 8.5 is more correct?

Some of the test config needs updating since #2865579: Rewrite the 'Latest revision' views filter and remove the revision_tracker table.

1. +++ b/core/modules/content_moderation/src/ViewsData.php
   @@ -79,7 +79,7 @@ public function getViewsData() {
   -        'filter' => ['id' => 'moderation_state_filter'],
   +        'filter' => ['id' => 'moderation_state_filter', 'allow empty' => TRUE],
   
   @@ -99,7 +99,7 @@ public function getViewsData() {
   -        'filter' => ['id' => 'moderation_state_filter'],
   +        'filter' => ['id' => 'moderation_state_filter', 'allow empty' => TRUE],
   

   This was assumed for views with a relationship, but now that we've removed the relationship seems like we have to put it back manually.

2. +++ b/core/modules/content_moderation/tests/modules/content_moderation_test_views/config/install/views.view.test_content_moderation_state_filter.yml
   @@ -254,16 +214,7 @@ display:
   -      relationships:
   -        latest_revision__node:
   -          id: latest_revision__node
   -          table: content_revision_tracker
   -          field: latest_revision__node
   -          relationship: none
   -          group_type: group
   -          admin_label: 'Content latest revision'
   -          required: true
   -          plugin_id: standard
   
   +++ b/core/modules/content_moderation/tests/src/Kernel/ViewsModerationStateFilterTest.php
   @@ -117,16 +117,6 @@ public function testStateFilterViewsRelationship() {
   -    // The latest revision for both nodes is a draft status.
   -    $this->assertNodesWithFilters([$node, $second_node], [
   -      'latest_revision_state' => 'draft',
   -    ]);
   -
   -    // Only node three has a published latest revision.
   -    $this->assertNodesWithFilters([$third_node], [
   -      'latest_revision_state' => 'published',
   -    ]);
   

   Since #2865579: Rewrite the 'Latest revision' views filter and remove the revision_tracker table we no longer have the latest_revision relationship, so no need to test it here.

According to #126 comment I have updated last patch. In this update was provided ability to select workflow types in ExtraOptionsForm and value options now related to this option, if nothing is selected it shows all the options.

Minor fix

I fully agree with #124 and the fixes from #131 makes sense, so back to RTBC!

@hamrant, I discussed #126 with @Sam152 and we both think that this extra feature is outside the 80% use case of this filter and that it should be discussed/added in a followup. We need to keep in mind that time is very important here because we still very much want this issue and the one that it blocks to be committed before 8.4.0-beta2.

Re-uploading the patch from #131 to be clear which one we think it's ready to go.

Thanks all! Assigning to myself for review both of the work on the issue here since and to test the kinds of edgecase scenarios I'm concerned about.

@amateescu, ok, thanks for checking