Problem/Motivation

While working on a site involving the contrib Workflow module, we noticed that workflow States were not being picked noticed as "enforceable" config forms. This appears to be due to the way the /admin/config/workflow/workflow/my_workflow/states form is built using a ConfigEntityListBuilder form builder class, which Config Enforce doesn't recognize as a config-related form.

Steps to reproduce

  1. Install a site with Config Enforce, Config Enforce Devel, and Workflow (contrib) modules
  2. Create a workflow entity (eg. my_workflow). Notice that Config Enforce will pick up the base entity
  3. Create some workflow States
  4. Notice that the Config Enforce UI is missing from the /admin/config/workflow/workflow/my_workflow/states page.

Workaround: We found that the State configs could still be enforced properly using Config Enforce Devel's "Generate from Active Storage" feature, under the Enforced Configs /admin/config/development/config_enforce/enforced_configs page. The issue is purely with the ConfigResolver class recognizing these State config entities on their config form page.

Proposed resolution

By extending ConfigResolver to be aware of ConfigEntityListBuilder-based config forms, we should be able to support recognizing any config entities on such a form by loading the list of entities from the FormBuilder, iterating over the config entities, and returning the list of their names.

Remaining tasks

  1. 🗸 Roll a patch to recognize ConfigEntityListBuilder forms in ConfigResolver
  2. Write a test case to validate the functionality

User interface changes

The Config Enforce Devel form will show up correctly on config forms based on ConfigEntityListBuilder.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#3 config_enforce-support_config_entity_list-3352380-1.patch1.75 KBspiderman

Issue fork config_enforce-3352380

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

spiderman created an issue. See original summary.

spiderman opened merge request !11

spiderman’s picture

spiderman
he/they
Primary language English
Location Halifax, NS
commented
StatusFileSize
new config_enforce-support_config_entity_list-3352380-1.patch1.75 KB

I've uploaded a static patch file based on the current state of the MR, for inclusion in composer.json as needed.

ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented
Assigned: spiderman » ambient.impact
ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented

After poking around in the code (both ours and core), I've come up with the following possibilities:

  1. Looking at the expanded class hierarchy for ConfigEntityListBuilder, it looks like this is used for a whole bunch of things in core, so one way we can test this is to install a few of the core modules that extend the class, and then navigate to each route that makes use of that; for example, the block module uses this for the block list admin page (admin/structure/block).
    • Pro: we can just use the existing implementations without writing our own, which is faster.
    • Con: it'll tie to our tests to specific implementations that may change in future core releases, thus breaking our tests.
  2. Create our own classes that extend ConfigEntityListBuilder in a test module and use those for our tests.
    • Pro: will reduce the chances that a core update will break tests.
    • Con: will take more work and more time to implement.

I propose we go with the first option initially, and open a follow up issue to implement the second method down the road.

ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented

Hmm, upon investigation, it seems a lot of the instances in core aren't actually forms but output as tables or similar structures, which means that we don't even end up with ConfigResolver being created on a lot of them and the few that it does end up getting created for don't actually get far enough to be identified as having a config entity list. The one that does seem to work is the block list. Will investigate further.

ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented
Assigned: ambient.impact » Unassigned
Status: Active » Needs review
ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented

Marking as ready so that tests can run.

ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented
Parent issue: » #3305516: Config Enforce roadmap
spiderman’s picture

spiderman
he/they
Primary language English
Location Halifax, NS
commented

This looks great to me! I think relying on a core module's (esp the venerable block.module) implementation here is reasonable, since any changes there should likely result in some kind of change in our module as well. We'd either want to support the new implementation and/or adapt the current one. This also saves us some boilerplate in the test code, which is a nice upside :)

I'd love to see a follow-on ticket to add tests for the other types of config ConfigResolver supports, namely "simple" config, basic Config Entities, and Plugin-based Config Entities. With the new Trait introduced in #3353064: Create a test trait to abstract repetitive UI checks/asserts, along with the example set here, these should be trivial to implement, but increase the value of our test suite quite a bit.

Thanks @Ambient.Impact!

  • spiderman committed 997ed297 on 1.0.x 
    Issue #3352380 by spiderman: Add support for ConfigEntityListBuilder...
ambient.impact’s picture

ambient.impact
they/them
Primary language English
Location Toronto
commented
Status: Needs review » Fixed

Has been merged!

  • spiderman committed 997ed297 on 2.0.x 
    Issue #3352380 by spiderman: Add support for ConfigEntityListBuilder...

Status: Fixed » Closed (fixed)

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