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

spiderman created an issue. See original summary.

ergonlogic’s picture

I've started working on this, and will shortly push a re-architecture branch for both modules.

In addition to the classes above, I think we may want to add EnforcementLevel and EnforcementLevelCollection classes.

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 the EnforcementLevel\ReadOnly::trigger('cache_rebuild'), EnforcementLevel\APIChangesOnly::trigger('cache_rebuild'), etc.

ergonlogic’s picture

This is largely done now. Expect an RC this week.

  • ergonlogic committed 19fdbc5 on 1.0.x
    Issue #3175060: Drop backward compatibility with old config style.
    
  • ergonlogic committed 1ad1141 on 1.0.x
    Issue #3175060: Refactor registry config schema.
    
  • ergonlogic committed 2e8dff8 on 1.0.x
    Issue #3175060: Refactor deletion of multiple enforced configs.
    
  • ergonlogic committed 52b3260 on 1.0.x
    Issue #3175060: Further refactoring and cleanup.
    
  • ergonlogic committed 5c053b7 on 1.0.x
    Issue #3175060: complete refactoring for efficient updates to enforced...
  • ergonlogic committed 61710c5 on 1.0.x
    Issue #3175060: Refactor registry config in TargetModuleBuilder.
    
  • spiderman committed 6314b7a on 1.0.x
    Issue #3175060: refactor update enforced config to handle moving paths...
  • ergonlogic committed 69d0cf0 on 1.0.x
    Issue #3175060: Move enforcement to new ConfigEnforcer class.
    
  • ergonlogic committed 7abd8e9 on 1.0.x
    Issue #3175060: Add EnforcedConfigCollection class and move initial...
  • ergonlogic committed 8614298 on 1.0.x
    Issue #3175060: Rename some methods for better clarity.
    
  • ergonlogic committed a22e87e on 1.0.x
    Issue #3175060: Set collection as property, to avoid unnecesary...
  • ergonlogic committed a5b78df on 1.0.x
    Issue #3175060: Call CRUD methods on EnforcedConfigCollection from...
  • ergonlogic committed ae93a8b on 1.0.x
    Issue #3175060: Standardize variable naming in mass CRUD methods.
    
  • ergonlogic committed b10ffc2 on 1.0.x
    Issue #3175060: Only update hashes after auto-import.
    
  • spiderman committed bd91c94 on 1.0.x
    Issue #3175060: add EnforcedConfigFile class, and refactor config...
  • ergonlogic committed c29a37b on 1.0.x
    Issue #3175060: Use mass update method from enforced configs form.
    
  • ergonlogic committed d67f31a on 1.0.x
    Issue #3175060: Further refactoring to better consolidate classes.
    
  • ergonlogic committed dbd90ac on 1.0.x
    Issue #3175060: Split config_devel integration into a stand-alone class.
    
  • ergonlogic committed e54e4ec on 1.0.x
    Issue #3175060: Refactor adding multiple enforced configs.
    
  • ergonlogic committed e6b7333 on 1.0.x
    Issue #3175060: Avoid duplicated object instantiation.
    

  • spiderman committed b1ce4e8 on 1.0.x
    Issue #3175060: refactor TargetModule and EnforcedConfigRegistry to...

  • spiderman committed dd2dc5a on 1.0.x
    Issue #3175060: refactor EnforcedConfigRegistry to no longer write...
  • spiderman committed f5b008b on 1.0.x
    Issue #3175060: do not write target_module in settings, now generated at...

  • spiderman committed 2ac4878 on 1.0.x
    Issue #3175060: derive config_file_path when loading...
  • spiderman committed 57340c5 on 1.0.x
    Revert "Issue #3175060: do not write target_module in settings, now...
  • spiderman committed 924336c on 1.0.x
    Revert "Issue #3175060: refactor TargetModule and EnforcedConfigRegistry...
  • spiderman committed c607a4f on 1.0.x
    Revert "Issue #3175060: refactor EnforcedConfigRegistry to no longer...
  • spiderman committed fb047b1 on 1.0.x
    Issue #3175060: derive target_module when loading EnforcedConfigRegistry...
spiderman’s picture

Status: Active » Fixed

This 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>.yml

enforced_configs:
  'config_enforce:registry:config_enforce_test':
    config_directory: config/install
    enforcement_level: 20
    config_form_uri: /admin/config/development/config_enforce/enforced_configs
    hash: intentionally_invalid
  'system:site':
    config_directory: config/optional
    enforcement_level: 20
    config_form_uri: /admin/config/system/site-information
    hash: 2z8CrotreUIXm3hgsJzMozVGOj82WJJXRRwZuQl9SlQ

So the name of the registry config object is now config_enforce.registry.<target_module> and we've eliminated the target_module and config_file_path keys entirely, replacing them with code that derives them when the registry is loaded. In particular, the config_file_path being derived means we shouldn't have issues with the target modules moving around within the web tree later on.

  • llamech committed 2536656 on 1.0.x
    Issue #3175060: Fix caching of target modules.
    

  • ergonlogic committed 007d1bb on 1.0.x
    Issue #3175060: Bail early on CRUD operations if no enforced configs are...
  • ergonlogic committed d0ba921 on 1.0.x
    Issue #3175060: Clean up autoImport().
    
  • ergonlogic committed eccc603 on 1.0.x
    Issue #3175060: Skip configs whose hashes have not changed in...

Status: Fixed » Closed (fixed)

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