Add support for ConfigEntityListBuilder config forms

Created on 5 April 2023, about 1 year ago
Updated 19 April 2023, about 1 year ago

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.

Feature request
Status

Fixed

Version

1.0

Component

Code

Created by

🇨🇦Canada spiderman Halifax, NS

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @spiderman
  • @spiderman opened merge request.
  • 🇨🇦Canada spiderman Halifax, NS

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

  • Assigned to Ambient.Impact
  • 🇨🇦Canada Ambient.Impact Toronto

    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.

  • 🇨🇦Canada Ambient.Impact Toronto

    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.

  • Issue was unassigned.
  • Status changed to Needs review about 1 year ago
  • 🇨🇦Canada Ambient.Impact Toronto

    Marking as ready so that tests can run.

  • 🇨🇦Canada spiderman Halifax, NS

    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+], 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...
  • 🇨🇦Canada Ambient.Impact Toronto

    Has been merged!

  • Status changed to Fixed about 1 year ago
    • spiderman committed 997ed297 on 2.0.x
      Issue #3352380 by spiderman: Add support for ConfigEntityListBuilder...
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.69.0 2024