Problem/Motivation

There are closely related bugs:

  1. ContentModerationStateFormatter::isApplicable() checks for a field name of moderation_state and does not include a check for the entity type, implying that it is to be used for any moderated entity. It assumes the existence of a workflow field on the parent entity, however, which is the case for the content_moderation_state entities, but not for the moderated entities. Because the moderation_state field of moderated entities is computed, it does not show up in the UI, so you cannot enable the formatter for moderated entities. If, however, you add a string field called moderation_state to any other entity type, however, you will be able to select this formatter and break your site.
  2. Drupal\content_moderation\ViewsData::getViewsData adds views data for the moderation_state to moderated entities specifying ContentModerationStateFormatter as the default formatter. This is broken in three ways:
    1. As explained above ContentModerationStateFormatter doesn't actually work for the moderated entity
    2. The views field info is missing id, entity_type and field_name so that instead of triggering an error in the formatter, you simply get a Broken/missing handler
    3. Because the moderation_state field of moderated entities is a computed field, there is no field storage definition for it, so that Drupal\views\Plugin\views\field\EntityField will actually fatal even if you provide the correct views data for it

Proposed resolution

  1. Add a check for the entity type to ContentModerationStateFormatter::isApplicable()
  2. Remove the incorrect views data for the moderation_state field from Drupal\content_moderation\ViewsData::getViewsData
  3. Possibly add ContentModerationStateFormatter as a default formatter to the view display options of the moderation_state base field definition of the content moderation state entity type. I presume this is what was actually the intention of the faulty views data.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#16 2845151-fix-formatter-12--test-only.patch2.36 KBSam152
#14 2845151-fix-formatter-14.patch5.77 KBSam152
#12 2845151-fix-formatter-12.patch9.57 KBSam152
#12 interdiff.txt877 bytesSam152
#10 Screen Shot 2017-02-13 at 7.15.20 PM.png20.91 KBSam152
#10 2845151-fix-formatter-10.patch6.06 KBSam152
#10 2845151-fix-formatter_PLUS_VIEWS-10.patch9.41 KBSam152
#10 Screen Shot 2017-02-13 at 6.26.14 PM.png20.81 KBSam152
#10 Screen Shot 2017-02-13 at 5.59.44 PM.png78.17 KBSam152
#6 interdiff.txt901 bytestimmillwood
#6 2845151-6.patch2.09 KBtimmillwood
#2 2845151-2.patch2.03 KBtimmillwood
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler created an issue. See original summary.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Active » Needs review
FileSize
2845151-2.patch2.03 KB

This patch should resolve the issues, but goes to show we really need more tests around the views integration.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Issue tags: +Workflow Initiative
jeqq’s picture

jeqq
Primary language Romanian
CreditAttribution: jeqq for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/content_moderation/src/Plugin/Field/FieldFormatter/ContentModerationStateFormatter.php
@@ -42,7 +44,7 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
-    return $field_definition->getName() === 'moderation_state';
+    return $field_definition->getTargetEntityTypeId() === 'content_moderation_state';

I think the field name check should stay, as well. Otherwise it would be available to any string field on the content moderation state entity.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
2845151-6.patch2.09 KB
interdiff.txt901 bytes

Fixing #5.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented
Status: Needs review » Reviewed & tested by the community

Perfect, thanks.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

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

Well the purpose of the formatter was to display the label of the state and not the machine name in views. We appear to have lost test coverage of this :(

So the change doesn't look quite right.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
Screen Shot 2017-02-13 at 5.59.44 PM.png78.17 KB
Screen Shot 2017-02-13 at 6.26.14 PM.png20.81 KB
2845151-fix-formatter_PLUS_VIEWS-10.patch9.41 KB
2845151-fix-formatter-10.patch6.06 KB
Screen Shot 2017-02-13 at 7.15.20 PM.png20.91 KB

I think this is two problems:

  • The formatter is broken.
  • Views support for computed fields is lacking.

I think I can fix the first issue with a plan to work on the second. Firstly, I think it makes the most sense for the computed field to actually have formatters and not the field on the content_moderation_state entity. It's a layer of indirection which is painful to traverse, simply not readily available in most view related contexts like manage display (I have a use case for the formatter in an inline entity form handler which allowed me to catch #2817565: Bugs in ModerationStateFieldItemList mean it can never be used with a field formatter). While it's not display configurable, this is pretty easily altered to produce something like the following:

Maybe an edge case, but still, using the correct APIs in general is a good idea IMO. We don't want to make the computed field an ugly step-child that doesn't have support for this kind of thing.

Part two of this looks like lacking support for computed fields. I think we can fix this, I've put together a prototype to demonstrate this working, but it would be a great addition to core, CM seems like a great use case to push it in. Attached is a patch which adds a 'computed' handler and uses it for the moderation field. Gives you something like:

It's a hack job right now and would of course need all of its own tests and validation to push through, but it shows that it's at least possible. Worst case in the interim, you could implement your own plugin to simply format $entity->moderation_state.

Status: Needs review » Needs work

The last submitted patch, 10: 2845151-fix-formatter-10.patch, failed testing.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Needs work » Needs review
FileSize
interdiff.txt877 bytes
2845151-fix-formatter-12.patch9.57 KB
timmillwood’s picture

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

I'm sure it used to be possible to create a view of "Content Moderation State" entities, which have a relationship back to the moderated node, allowing a list of, for example, all entities (of any entity type) with the moderation state "draft", but I can't seem to do this with HEAD or any of the patches in the this issue.

Also, I think from #10 we are going out of scope of the original issue. In HEAD we can create a view of Nodes, add a relationship to Content Moderation State, and it can list the moderation state label (not machine name). All this issue should do is clean up \Drupal\content_moderation\Plugin\Field\FieldFormatter\ContentModerationStateFormatter::isApplicable and add a test.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Needs work » Needs review
FileSize
2845151-fix-formatter-14.patch5.77 KB

The views stuff is purely for sake of interest, shouldn't have been included in the latest patch. I created the followup ages ago #2852067: Add support for rendering computed fields to the "field" views field handler.

The norm for creating views is to use the base table as you would normally or the relationship through the revision tracker table. Not sure creating a view of "Content Moderation State" is ever really useful.

The real case I tried to make with #10 was that the formatter should work against the computed field, not the field on the related entity. If this approach is accepted, there is a test in the latest patch.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

ok, looks good.

Tested a "test only" version of #14 locally, and it fails, which is right.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
FileSize
2845151-fix-formatter-12--test-only.patch2.36 KB

For posterity.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 2845151-fix-formatter-12--test-only.patch, failed testing.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs work » Reviewed & tested by the community

Test only patch failed!

Back to RTBC for #14.

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 0d27b06 to 8.4.x and 076c573 to 8.3.x. Thanks!

  • alexpott committed 0d27b06 on 8.4.x 
    Issue #2845151 by Sam152, timmillwood, tstoeckler:...

  • alexpott committed 076c573 on 8.3.x 
    Issue #2845151 by Sam152, timmillwood, tstoeckler:...

Status: Fixed » Closed (fixed)

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