Problem/Motivation
When content_translation is enabled, it uses the BaseFieldOverride
entity in order to mark specific fields as translatable/untranslatable. When the moderation_state field has an override for anything, not just translation status, it stops working properly. Instead of an instance ModerationStateFieldItemList
, we are given a FieldItemList
for the moderation_state
field.
Proposed resolution
Fix BaseFieldOverride
so that it returns the correct list class.
Remaining tasks
Commit!
User interface changes
None.
API changes
Updated return value from BaseFieldOverride::getClass
to match return value of the BaseFieldDefinition
the BaseFieldOverride
was created for.
Data model changes
None.
Steps to reproduce
Original issue manifested in conflict between content_moderation and content_translation:
- Install Drupal with the standard profile
- Enable Content Moderation and Content Translation modules
- Enable "Editorial workflow" for the Article content type.
- Add a new Article. Save it as draft, and then edit it again and save the changes as draft.
There won't be any errors and the changes will be saved - Add an additional language and enable translation for the Article node, and only for that type.
- Add an another test article again, repeat 4th step (above).
Exception will be thrown - Now repeat the 4th step with the Basic page content type as well
Exception will be thrown
Comment | File | Size | Author |
---|---|---|---|
#55 | basefieldoverride-2848775-55.patch | 574 bytes | SurfinSpirit |
#45 | 2848775-45.patch | 4.02 KB | Sam152 |
#45 | 2848775-45_TESTS_ONLY.patch | 3.46 KB | Sam152 |
#45 | interdiff.txt | 2.89 KB | Sam152 |
#39 | 2848775-39.patch | 2.07 KB | Sam152 |
Comments
Comment #2
huzookaComment #3
asghar CreditAttribution: asghar commentedHi @huzooka
I have repeated the same steps for Drupal version 2.5 and 8.2.6 on local and simplytest.me respectively but could not produce the error. In your 3rd step you mentioned *Editorial workflow* it meant *Manage moderation* Dropdown right?. Could you produce the same error on simplytest.me ?. Thanks
Comment #4
huzookaHi @asghar,
This issue is about Drupal 8.4.x and Drupal 8.3.x. The referred issue targets Drupal 8.2.x, but I'm almost sure that one won't be fixed.
Comment #5
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI could not reproduce this in 8.4.x HEAD.
Comment #6
stmh CreditAttribution: stmh at Factorial GmbH commentedI can reproduce this issue with 8.3.2, enabling content_translation and content_moderation. Saving a second, unpublished draft will throw this error.
Comment #7
D34dMan CreditAttribution: D34dMan as a volunteer and commentedThe patch attempts to check if the moderation state value exists before trying to use them.
Comment #8
D34dMan CreditAttribution: D34dMan as a volunteer and commentedJust in case anybody wants to try the patch with 8.3.2, attaching for the sake of convenience.
Comment #9
D34dMan CreditAttribution: D34dMan as a volunteer and commentedThis work includes translation into consideration
Comment #10
D34dMan CreditAttribution: D34dMan as a volunteer and commentedSorry, about the previous patch, fixed the error in above patch int his one.
Comment #11
D34dMan CreditAttribution: D34dMan as a volunteer and commentedAttaching fix for 8.4.x-dev
Comment #12
D34dMan CreditAttribution: D34dMan as a volunteer and commentedIn my local install based on 8.3.2, there were lots of revisions for a particular node and its translation, before the content moderation and workflow as enabled. Also, none of the nodes were in published state either. After enabling content moderation and workflow, I assigned the editorial workflow to these contents. After this, attempting to save the nodes started giving Exceptions. Applying the patch solves the issue for me.
Comment #13
stmh CreditAttribution: stmh at Factorial GmbH commentedPatch works on my end
Comment #16
eyilmazThe getTranslation() method throws an exception when there is no translation.
Check for hasTranslation before executing getTranslation().
Patches for 8.3.x & 8.4.x attached.
Comment #17
eyilmazComment #20
D34dMan CreditAttribution: D34dMan as a volunteer and commented@eyilmaz that makes sense
Comment #21
eyilmazUpdating patch again.
The last return in isDefaultRevisionPublished is checking for the wrong revision.
Comment #22
eyilmazFixing another issue where original_id was always set.
Comment #23
eyilmazComment #25
timmillwoodI still can't reproduce the issue.
Comment #26
catchComment #27
catchI think this might be a duplicate of an older issue that was since fixed, but can't find that issue at the moment.
Comment #28
eyilmazRoot cause for this may be that there is a fieldoverride configuration for the moderation_state field. Then ModerationStateFieldItemList is not used when trying to get the value for moderation_state (since type is string). This is for 8.3. did not checked for 8.4.
Comment #29
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI tried to reproduce this, but couldn't trigger the exact exception. I did run into some weird scenarios which I think could be the same issue reported here, but manifesting in different ways.
I was able to reproduce something similar with:
drush si standard -y && drush en content_moderation -y && drush en content_translation -y
/admin/config/regional/language
/admin/config/regional/content-language
/node/add/page
. The widget isApplicable method returns FALSE and a textfield widget is shown:article
, but never onpage
again. The textfield sticks around and can be used to enter invalid states which trigger a very similar error message in the IS.The state 'foo' does not exist in workflow
Comment #30
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI spent some more time debugging this. I think #28 is correct and any field config override breaks moderation in the exact way the issue summary describes.
Content translation simple exposes this in
content_translation_form_language_content_settings_submit
:The callstack I get with this test-only patch is:
Note: I don't think the translation status of the field matters in any of these test cases, it's irrelevant given it's a computed field with an always-translatable entity backing it.
#29 is a seperate issue which I'll open shortly, with the same cause, a field config override.
Comment #31
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI've logged #2901478: Test ModerationStateWidget::isApplicable with a BaseFieldOverride for #29. Looking at a fix for ##30.
Comment #33
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedEdit: This patch name is incorrect, this is the test and fix ^
Tracked the instantiation of the FieldListItem class through
TypedDataManager
to arrive atBaseFieldOverride
returning the wrong class for::getClass()
.I think this possibly points to a broader problem, the
BaseFieldOverride
entity does not work properly with computed fields with overriden list classes. The parent implementation of::getClass
this was falling back to looks like:Which would make for configuration based fields, you can't change the list class, it's always based on the field type. But in the override use case, calling the storage definitions implementation of
getClass
is more comprehensive because it checks the definition for a class before calling back off toplugin.manager.field.field_type
:The fix in the patch attached is in-line with other methods which presumably are designed to solve similar problems:
Lets see what the testbot thinks.
Comment #34
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI hope it's not too presumptuous of me to update the IS and title based on what I've found. With the patch in #33 applied, I can no longer reproduce the error which I was seeing consistently following the steps in the issue summary.
Comment #35
DuneBLI have rerolled #22 for 8.5
But I discover that, in 8.5, workflow::getState() doesn't exists anymore!!!
It has been moved into a WorkflowTypeBase.
This is why I have created a very ugly patch which will work only if the user use the default state of the module.
This is a patch that can be used only to make your site working quickly (and a proof of concept that #22 is doing the job)
Comment #36
DuneBL@Sam152: sorry I have posted #35 without testing/reading #30->#35
Comment #37
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedNo problem! If you are encountering the problem, some testing of #33 would be greatly appreciated.
Comment #38
DuneBLI confirm that #33 is working nicely!!!
Many thanks
Just a small offset in 8.5
Comment #39
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedMinor change to bring the method in-line with the others.
Comment #43
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe analysis in #33 and the fix makes sense indeed, but I'm concerned that I couldn't find any kernel or unit test for the
BaseFieldOverride
entity type. Wouldn't it be better to add a generic test for that instead of the Content Moderation one?But if we decide to keep this test as well, I just one remark for it:
How about testing the reverse scenario: don't provide any value for the
moderation_state
field, check the 'draft' default value has been applied, and then change it to published? I think that's how we usually test our computedmoderation_state
field :)Comment #44
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI agree, we should have a dedicated test for this. The issue where
BaseFieldOverride
was introduced included a bunch of unit tests for this stuff, but I can't find where they are in HEAD.Comment #45
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI couldn't find an existing test class where this made sense, I've created a new one specifically for testing the
BaseFieldOverride
entity.Comment #47
timmillwoodCan't see any reason to not RTBC.
Yet again, nice investigation from @Sam152!
I guess we should update the issue summary based on #33?
Also, does this need a change record, not that it's a change, but it's a pretty deep and weird bug fix.
Comment #48
catchI don't think this needs a change record since no-one should have to do anything differently, but definitely the issue summary update, tagging for a release notes mention.
Patch looks very straightforward but didn't do an in-depth review of the tests or anything yet.
Comment #49
plachRTBC +1
Minor stuff that can be fixed on commit:
Tests
@#43:
A standalone test works for me, however the original kernel test is in
EntityFieldTest
.Comment #50
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #53
catchGiven the tests are installing different modules/schema I think the standalone test class is OK.
Fixed the comment issue on commit.
Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!
Comment #54
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedWoohoo! Any WI critical squashed! Thanks for the attention you are giving these issues @catch.
Comment #55
SurfinSpirit CreditAttribution: SurfinSpirit at FFW commentedKernel tests are not applying on 8.3.4 so attaching a patch without tests.