Problem/Motivation
In trying to tackle #3172512: Implement config schema, we recognized that the way we store config "registries" should be refactored to conform more to standard convention for config objects, where the top-level structure is a mapping (key -> value) rather than a simple unkeyed array.
In considering how to refactor the code to make that change, we realized that our recent performance improvements in #3171651: Improve performance pointed the way to a better organization of the class structure to better support what the module is doing:
* EnforcedConfig - simple data object, just keeps the settings of enforcement (level, hash, name)
* TargetModule - a single module where enforced config can live
* EnforcedConfigRegistry - set of EnforcedConfig objects' settings, specific to 1 TargetModule
* EnforcedConfigCollection - collection of all EnforceConfigRegistry objects, for acting on them en masse
* TargetModuleCollection - ensure a default TargetModule exists, and allow EnforcedConfigCollection to gather all available TargetModules
With these conceptual pieces, we can reorganize and better encapsulate each one's logic, and clean up the public-facing API for the module as a whole:
* TargetModule will contain the single EnforcedConfigRegistry for that module, which encapsulates the logic of reading/writing the (renamed) `config_enforce.registry.[targetmodule].yml` config objects/files.
* EnforcedConfigRegistry in turn contains a number of EnforcedConfig data objects that track the "settings" for each config object being enforced (config name, directory, enforcement level, hash, and optional config_form_uri)
This should allow us to streamline things and still support a) enforcing single config objects via their primary configuration form, which is the primary way of using Config Enforce, as well as b) acting upon multiple enforced configs at once, as in the case of "generate from module", "generate from active config", "update settings", or mass deletion. It also appears to simplify the data we need to track, and still allow performant operation.
This entails some heavy refactoring of the code, for which we'd prefer to wait until we have a reasonable test suite in place, to ensure the existing functionality doesn't regress.
Remaining tasks
* Build out test suite to cover key functionality of the modules (#3174603: Add automated tests)
* Construct EnforcedConfigCollection class to handle mass Registry management
* Factor our EnforcedConfig save() and related methods into EnforcedConfigRegistry
* Clean things up further?
* PROFIT!
API changes
Not entirely clear as yet, but the goal is to better encapsulate the logic and clarify the APIs between the classes described above.
Data model changes
We imagine the structure of the config_enforce.enforced_configs.[targetmodule] config object to change from:
system__site:
target_module: my_site_module
config_directory: config/install
config_file_path: modules/custom/my_site_module/config/install/system.site.yml
enforcement_level: '20'
config_form_uri: /admin/config/system/site-information
hash: r_sS8qvN4H7zzH8Z9MAnnroYoytV15MF5bGUxyQZuGE
To something more like (and renamed to config_enforce.registry.[targetmodule]
target_module: my_site_module
enforced_configs:
- config_name: system.site
config_directory: config/install
enforcement_level: '20'
config_form_uri: /admin/config/system/site-information
hash: r_sS8qvN4H7zzH8Z9MAnnroYoytV15MF5bGUxyQZuGE
Comments
Comment #2
ergonlogicI've started working on this, and will shortly push a
re-architecturebranch for both modules.In addition to the classes above, I think we may want to add
EnforcementLevelandEnforcementLevelCollectionclasses.This would allow us to delegate the actual enforcement to a given class, rather than embedding that logic wherever it's being triggered.
For example, we could then have our cache rebuild hook call something like
EnforcementLevelCollection::trigger('cache_rebuild'), which would in turn call theEnforcementLevel\ReadOnly::trigger('cache_rebuild'),EnforcementLevel\APIChangesOnly::trigger('cache_rebuild'), etc.Comment #3
ergonlogicThis is largely done now. Expect an RC this week.
Comment #8
spidermanThis is pretty much done, although we didn't fully flesh out the test suite per #3174603: Add automated tests, we at least got a basic config/schema test in place per #3172512: Implement config schema.
In the end, we simplified the format of the config registry down to the following:
File:
config_enforce.registry.<target-module>.ymlSo the name of the registry config object is now
config_enforce.registry.<target_module>and we've eliminated thetarget_moduleandconfig_file_pathkeys entirely, replacing them with code that derives them when the registry is loaded. In particular, theconfig_file_pathbeing derived means we shouldn't have issues with the target modules moving around within the web tree later on.