Problem/Motivation

The Content Moderation module is not tested with an entity type that is not bundleable. We should add a test to ensure it works. The test should include uninstalling the module that provides the entity type to ensure dependencies are managed correctly.

An example of a content entity type that is not bundleable is User. At the moment content_moderation is tied to revisionability. I'm not completely sure why. Maybe we should explore disentangling that here.

Proposed resolution

We only have tests for entity types with non config entity bundles. Given there is no entity test type with no bundle our total coverage in this area is probably quite limited. We'll need to add a test type as far as I can see.

Remaining tasks

User interface changes

None

API changes

Data model changes

None

CommentFileSizeAuthor
#12 2850353-test-with-no-bundle-6.patch3.29 KBtimmillwood
#7 2850353-test-with-no-bundle-7.patch2.35 KBPavan B S
#6 2850353-test-with-no-bundle-6.patch3.29 KBsam152

Comments

alexpott created an issue. See original summary.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented

If we remove the dependency of revisions it won't be possible to have a draft of a published entity, aka "forward revision".

We could have a warning on non-revisionable entity forms to say "setting this to draft will unpublish the entity".

amateescu’s picture

amateescu commented

We could have a warning on non-revisionable entity forms to say "setting this to draft will unpublish the entity".

However, this would defeat the purpose of Content Moderation and its concept of multiple states, no? :)

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented

However, this would defeat the purpose of Content Moderation and its concept of multiple states, no? :)

You could say the same about only supporting EntityPublishedInterface entities.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

You can have multiple states without revisions. We have published and unpublished in core without revisions. You only need revisions for forward revisions. As far as i can see, content moderation does have to have forward revisions.

sam152’s picture

sam152 commented
Status: Active » Needs review
StatusFileSize
new 2850353-test-with-no-bundle-6.patch3.29 KB

Attached is a test. From what I can see, leaving off the "bundle" entity key is enough to ensure there is no bundle added, even when extending EntityTest. It defaults the bundle to the same name as the entity type ID for most purposes, except for when creating the entity fields.

The issue for removing the dependency on revisions is here #2850627: Do not require a revisionable entity type when using content_moderation..

Pavan B S’s picture

Pavan B S commented
StatusFileSize
new 2850353-test-with-no-bundle-7.patch2.35 KB

Rerolled the patch.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented

Did it really need a re-roll? and looks like the new patch is now missing a main part?

Looks like a credit mining attempt to me.

Leaving as "Needs review" because #6 still seems valid. Reviews are often much more helpful than broken patches.

damiankloip’s picture

damiankloip commented

Yes - It's missing the main part, and doesn't have the correct changes for the test assertions either. So it's not a reroll anyway, it's a re-make... Either way, the patch from #6 still APPLIES FINE.

These cheap commit mining attempts need to stop. It holds up issues, confuses people, as well as holding up the test bot when it could be testing useful patches.

Valuebound --

Status: Needs review » Needs work

The last submitted patch, 7: 2850353-test-with-no-bundle-7.patch, failed testing.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented
Status: Needs work » Needs review
timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 2850353-test-with-no-bundle-6.patch3.29 KB

Re-uploading #6 for clarity.

Suggested commit:

git commit -m 'Issue #2850353 by Sam152: Test content_moderation with a non-bundleable content entity' --author="sam <sam@1485048.no-reply.drupal.org>"
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
+++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php
--- /dev/null
+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestNoBundle.php

I was going to say: surely there already is such an test entity type… but no, there isn't! Great addition :)

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented

@Wim Leers - I think everyone has been too dependent on copy&paste when creating new entity_test entity types.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 0aa52f7 to 8.4.x and 8104932 to 8.3.x. Thanks!

As per discussion on the issue I did not credit @Pavan B S.

  • alexpott committed 0aa52f7 on 8.4.x 
    Issue #2850353 by timmillwood, Pavan B S, Sam152: Test...

  • alexpott committed 8104932 on 8.3.x 
    Issue #2850353 by timmillwood, Pavan B S, Sam152: Test...

Status: Fixed » Closed (fixed)

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