Problem/Motivation

Setting an invalid moderation state on a moderated entity fails, but this is not tested..

Proposed resolution

Add test coverage.

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#4 2845113-4.patch1.06 KBtimmillwood
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented

Note that there is ModerationStateConstraintValidator which is related. Not sure if it could be re-used, but maybe yes.

Sam152’s picture

Sam152 CreditAttribution: Sam152 at PreviousNext commented

It's a computed field which does actually get looked up from a workflow when being set and saved, so I would actually suspect that it's impossible right now to set it to something invalid.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Active » Needs review
Issue tags: +Workflow Initiative
FileSize
2845113-4.patch1.06 KB

Right, it is not possible to set moderation state to an invalid state, for example $entity->moderation_state->value = 'foobar'; $entity->save(); throws 

EntityStorageException: The state "foobar" does not exist in
workflow "editorial"

Maybe we need a test for this?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented
Title: Add a constraint to the content moderation states that enforces a valid workflow state » Add test coverage that setting an invalid moderation state on a moderated entity fails
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Ahhh, I see we end up getting into content_moderation_presave() / EntityOperations::entityPresave() which does $workflow->getState($entity->moderation_state->value) and that throws the exception.

Very interesting. Given that I do think it's worth having this test coverage, but it seems pointless to add an additional constraint, so re-titling. Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 2845113-4.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

Unrelated test fail.

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.

cilefen’s picture

cilefen CreditAttribution: cilefen commented

Updating credit.

  • cilefen committed 95e9b9e on 8.4.x 
    Issue #2845113 by timmillwood, tstoeckler: Add test coverage that...

  • cilefen committed 9bb4595 on 8.3.x 
    Issue #2845113 by timmillwood, tstoeckler: Add test coverage that...
cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Reviewed & tested by the community » Fixed

Committed 95e9b9e and pushed to 8.4.x and cherry-picked as a straightforward testing improvement to 8.3.x as 9bb4595. Thanks!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 8.4.x-dev » 8.3.x-dev

+1 for the backport as a testing improvement and experimental module. This looks good to me too.

Status: Fixed » Closed (fixed)

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