Problem/Motivation

Workflows has a ConfigImportSubscriber that runs on all config imports.
It assumes that all config imported will be workflow entities.

Proposed resolution

Only process config that corresponds to a workflow entity.

Remaining tasks

Possibly rewrite the test class to test each type of import (simple config, non-workflow entity, workflow entity) separately

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#5 2900019-workflows-5-PASS.patch2.86 KBtim.plunkett
#5 2900019-workflows-5-FAIL.patch1.48 KBtim.plunkett
#3 getentitytypeidbyname-2900019-3.patch1.9 KBjoelpittet
#3 interdiff.txt1.27 KBjoelpittet
#2 2900019-workflow-2.patch1.52 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented
Priority: Normal » Major
Status: Active » Needs work
Issue tags: +Needs tests
FileSize
2900019-workflow-2.patch1.52 KB

This fixes the bug, mirroring the checking of the return value done in \Drupal\Core\Config\ConfigImporter.

However, I don't understand this method (and class) at all.

It runs for ALL entity types, and the @return indicates that.
Yet the calling code above it calls WorkflowInterface-specific methods on it:

              $state = $workflow->getTypePlugin()->getState($state_id);

Above that, I'd imagine this line is throwing undefined index errors:

            $diff = array_diff_key($workflow_config['type_settings']['states'], $original_workflow_config['type_settings']['states']);

Plus, as @joelpittet points out, this was running on all config data, including simple config, producing the above error.

It seems to me that the class was manually tested against workflow-only changes, and not when any other config data was being imported.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented
FileSize
interdiff.txt1.27 KB
getentitytypeidbyname-2900019-3.patch1.9 KB

Didn't have time to look into the tests but ran into more of what @tim.plunkett was talking about where the contact entity fatals in ConfigImportSubscriber::onConfigImporterValidate() when trying to call getTypePlugin().

Added a check for WorkflowInterface, this still needs tests but I'm going on vacation for a week so not likely to tackle those.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented

Oh and I got those undefined index errors too that was mentioned in #2

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2900019-workflows-5-FAIL.patch1.48 KB
2900019-workflows-5-PASS.patch2.86 KB

Here's a solution with less overhead. It also neatly solves both bugs, which I've included test coverage for.

The last submitted patch, 5: 2900019-workflows-5-FAIL.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented
Title: getEntityTypeIdByName can return NULL » \Drupal\content_moderation\EventSubscriber\ConfigImportSubscriber causes errors when importing any non-workflow config
Issue summary: View changes
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented
Status: Needs review » Reviewed & tested by the community

Thanks for adding a test, glad you found a way to speed up the check to avoid loading the entity.

  • larowlan committed 2534957 on 8.5.x 
    Issue #2900019 by tim.plunkett, joelpittet: \Drupal\content_moderation\...

  • larowlan committed 39831c8 on 8.4.x 
    Issue #2900019 by tim.plunkett, joelpittet: \Drupal\content_moderation\...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed as 2534957 and pushed to 8.5.x.

Cherry-picked as 39831c8 and pushed to 8.4.x.

Thanks for moving on this so fast.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

Thanks for fixing this!

I'm a little surprised the original patch didn't cause any core test fails. I guess we don't have extensive enough tests for config imports?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

I guess we don't have extensive enough tests for config imports

...while workflow module is enabled.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

...while workflow module is enabled.

Ah ha, yes, that is the key. Maybe we need a test that installs all modules then tests config import.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented

#14 sounds like a good idea @timmillwood.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer commented

\Drupal\KernelTests\FileSystemModuleDiscoveryDataProviderTrait might be handy for a test like that.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented

Created the follow-up: #2903205: Add Config Import Test with all modules enabled

Status: Fixed » Closed (fixed)

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