Define hooks to provide 'auto_import' and 'auto_export'

I will work on it.

Providing updated patch and interdiff file.

I'm not sure what the use case is for this.

What's the point in defining the config dynamically rather than in settings?

@joachim Apologies if the issue doesn't make it clear, but our use-case here is for Config Enforce → and Config Enforce Devel → modules, which keep their own registry of what configs to auto-import and auto-export. Rather than re-implement the functionality provided by Config Devel, we figured adding these hooks would more generically achieve the goal.

I'm happy to answer other questions about this, but fwiw we've been using this patch to allow Config Enforce to mature over the last couple years, and it's working very well for us.

Thanks for the explanation.

I'm not convinced by the architecture of using a hook -- the hook returns a list of EVERYTHING which then has to be searched for the CURRENT THING. But I don't know what the other side of the hook looks like -- maybe it's better DX for the hook inventor to do the filtering, rather than the hook implementor.

I'm happy to commit this but there's some things to clean up.

1. +++ b/config_devel.api.php
   @@ -0,0 +1,47 @@
   +
   

   Needs a @file docblock.

2. +++ b/config_devel.api.php
   @@ -0,0 +1,47 @@
   + * Provide Config Devel auto-import configs.
   

   Wrong tense.

3. +++ b/config_devel.api.php
   @@ -0,0 +1,47 @@
   + * @see ConfigDevelAutoImportSubscriber::autoImportProviderConfig().
   

   Should be fully qualified.

4. +++ b/config_devel.api.php
   @@ -0,0 +1,47 @@
   +  return $auto_import;
   

   Return not docu,ented.

5. +++ b/config_devel.api.php
   @@ -0,0 +1,47 @@
   + * React to a Config Devel auto-import (eg. save file hashes).
   

   Wrong tense.

6. +++ b/config_devel.api.php
   @@ -0,0 +1,47 @@
   + *   An array of configs and their respective files and hashes.
   

   This sounds like it needs more explanation. Where are the files and hashes in the array? Where are the configs?

7. +++ b/config_devel.api.php
   @@ -0,0 +1,47 @@
   + * @see hook_config_devel_auto_import() {
   

   Typo - '{' and missing full stop.

8. +++ b/config_devel.api.php
   @@ -0,0 +1,47 @@
   + * Provide Config Devel auto-export paths.
   

   Same.

9. +++ b/config_devel.api.php
   @@ -0,0 +1,47 @@
   + * @see ConfigDevelAutoExportSubscriber::autoExportProviderConfig().
   

   Same.

10. +++ b/config_devel.api.php
    @@ -0,0 +1,47 @@
    +  return $auto_export;
    

    Undocumented.

11. +++ b/src/EventSubscriber/ConfigDevelAutoExportSubscriber.php
    @@ -85,6 +96,22 @@ class ConfigDevelAutoExportSubscriber extends ConfigDevelSubscriberBase implemen
    +   * Automatically export configuration provided by other modules.
    

    Wrong tense.

12. +++ b/src/EventSubscriber/ConfigDevelAutoImportSubscriber.php
    @@ -24,6 +51,20 @@ class ConfigDevelAutoImportSubscriber extends ConfigDevelSubscriberBase implemen
    +   * Reinstall changed config files provided by other modules.
    

    Wrong tense.

Charlie ChX Negyesi → made their first commit to this issue’s fork.

Glad to see movement on this. Going to pitch in with some dependency injection and code quality fixes to the issue fork.

Added a commit to inject the module handler service. I added it to the two event subscribers and not the base class because technically only the event subscribers that extend ConfigDevelSubscriberBase use the module handler service, but I can see an argument for moving that to the base class if you all think it best.

I haven't gotten to the changes needed in config_devel.api.php but planning to a bit later today.

Alright, I've tried to document the hooks as best as I can but I'm not particularly familiar with the internals of this module so one someone will have to take a look and give me feedback.

@joachim Can you be more specific which words are using the wrong tense? The grammar might be a bit awkward, but I'm not really seeing any plural or singular where they feel wrong.

Opening merge request so testbot will run (hopefully). Also hiding patches in favour of the issue fork.

Hmm, looks like your testing config is out of date due to PHP 7.3 and core 8.9 no longer being supported. Test would have failed regardless, and I should have run it locally first to check. I'm reasonably familiar with Kernel and Functional tests but mocking up services is one area I haven't done much in so let me know if I'm missing something in the test I updated.

Also, I'm guessing we should create tests to test the hooks specifically?

Derp.

It took a few hours, but I reverse engineered the existing event subscriber tests into a working test for the import and export hooks. I haven't added testing for the post import hook so I'll try and get that done tomorrow.

This was made more challenging because the tests have a lot of undocumented things going on; there's a lot I could figure out easily, but a bunch of things that I can only guess at the purpose of; for example from ConfigDevelSubscriberTestBase:

    for ($i = 2; $i; $i--) {
      $data['label'] = $this->randomString();
      file_put_contents($filename, Yaml::encode($data));
      // The import fires an export too.
      $subscriber->autoImportConfig();
      $this->doAssert($data, Yaml::decode(file_get_contents($exported_filename)));
    }

Why is this in a loop? It's basically doing the exact same thing twice, with the only difference being the random string in the data - I assume this is the reason but for all I know there's more to it that I'm unaware of.

Just because something seems clear to you when you write it without comments doesn't mean it'll be clear at all to someone coming in a year or two down the line. If someone as experienced with writing complex systems like me is struggling to understand your code, it's going to be even worse for someone less experienced and may put them off from trying to contribute to begin with.

Had some time to include the post auto import hook in the test. Still needs a bit more documentation but it's like 95% of the way there.

I realize I haven't posted an update here but had my laptop stolen nearly two weeks ago so been pretty busy recovering from back ups and setting stuff up on the new laptop. I'm hoping to expand this test to specifically ensure the hook input and output conform to the expected formats.

Just pushed a commit that adds a new service to the hook test module. This service is overkill for the current hook test as it is, but it's intended to allow for varying the data to test for various failure states, which I'll be adding tomorrow or on a subsequent day.

Added a bunch more test data and tests for malformed data.

I'm almost done but need to add a test or two for the auto export hook, which I'm hoping to do tomorrow.

Alright, I think I'm done with this in both senses: there are now 9 tests for various success and failure states, including data validation, and it's well documented and explains why things are implemented the way they are; also, my brain is tired of looking at this code. 😂

Adding patch for easy linking to with the merge request state at this point in time. This is primarily intended just for the Composer patches plug-in; the actual merge request is what should be merged. Yes, I know merge requests provide a patch link but that may change as new commits are added, potentially breaking expectations of a project that links to that, so this serves as a snapshot. If anyone knows how to accomplish this without manually uploading a patch, feel free to let me know.