Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
There are closely related bugs:
ContentModerationStateFormatter::isApplicable()
checks for a field name ofmoderation_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 aworkflow
field on the parent entity, however, which is the case for thecontent_moderation_state
entities, but not for the moderated entities. Because themoderation_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 calledmoderation_state
to any other entity type, however, you will be able to select this formatter and break your site.Drupal\content_moderation\ViewsData::getViewsData
adds views data for themoderation_state
to moderated entities specifyingContentModerationStateFormatter
as the default formatter. This is broken in three ways:- As explained above
ContentModerationStateFormatter
doesn't actually work for the moderated entity - The views field info is missing
id
,entity_type
andfield_name
so that instead of triggering an error in the formatter, you simply get a Broken/missing handler - Because the
moderation_state
field of moderated entities is a computed field, there is no field storage definition for it, so thatDrupal\views\Plugin\views\field\EntityField
will actually fatal even if you provide the correct views data for it
- As explained above
Proposed resolution
- Add a check for the entity type to
ContentModerationStateFormatter::isApplicable()
- Remove the incorrect views data for the
moderation_state
field fromDrupal\content_moderation\ViewsData::getViewsData
- Possibly add
ContentModerationStateFormatter
as a default formatter to the view display options of themoderation_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
Comment | File | Size | Author |
---|---|---|---|
#16 | 2845151-fix-formatter-12--test-only.patch | 2.36 KB | Sam152 |
#14 | 2845151-fix-formatter-14.patch | 5.77 KB | Sam152 |
#12 | 2845151-fix-formatter-12.patch | 9.57 KB | Sam152 |
#12 | interdiff.txt | 877 bytes | Sam152 |
#10 | Screen Shot 2017-02-13 at 7.15.20 PM.png | 20.91 KB | Sam152 |
Comments
Comment #2
timmillwoodThis patch should resolve the issues, but goes to show we really need more tests around the views integration.
Comment #3
timmillwoodComment #4
jeqqComment #5
tstoecklerI think the field name check should stay, as well. Otherwise it would be available to any string field on the content moderation state entity.
Comment #6
timmillwoodFixing #5.
Comment #7
tstoecklerPerfect, thanks.
Comment #9
alexpottWell 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.
Comment #10
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI think this is two problems:
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.
Comment #12
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #13
timmillwoodI'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.Comment #14
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThe 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.
Comment #15
timmillwoodok, looks good.
Tested a "test only" version of #14 locally, and it fails, which is right.
Comment #16
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedFor posterity.
Comment #18
timmillwoodTest only patch failed!
Back to RTBC for #14.
Comment #19
alexpottCommitted and pushed 0d27b06 to 8.4.x and 076c573 to 8.3.x. Thanks!