- 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:
- 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.
- 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.
- Looking at the expanded class hierarchy for
- 🇨🇦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 2 years ago 10:33pm 8 April 2023 - 🇨🇦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...
-
spiderman →
committed 997ed297 on 1.0.x
- Status changed to Fixed
almost 2 years ago 8:11pm 19 April 2023 -
spiderman →
committed 997ed297 on 2.0.x
Issue #3352380 by spiderman: Add support for ConfigEntityListBuilder...
-
spiderman →
committed 997ed297 on 2.0.x
Automatically closed - issue fixed for 2 weeks with no activity.