Make all available tests pass

Could you, please, add more details about your case? Steps to reproduce, etc. The actual description is too generic.

I'm having the same problem.

On config export, the config is ignored, but on config import, my config is removed.

My config_ignore.settings.yml looks like this:

ignored_config_entities:
  - 'webform.webform.*'

When executing drush cim my webforms are removed.

The issue started since this specific commit, which was added in #3117135: TypeError: Argument 1 passed to NestedArray::unsetValue() must be of the type array, null given: https://git.drupalcode.org/project/config_ignore/-/commit/1c179cb78ce963...

While using 3.x-dev, I found that on config_import, all the ignore config names aren't fetched appropriately. While debugging, I found that this behaviour is because we're looping over config list ($transformation_storage->listAll()) of transformation storage and finding their intersection with ignored_config_etities. However, there might be some configs that're in the destination/sync storage but not in the active storage, due to which we don't fetch the complete list of configs to be ignored.

Here's the patch and it's working for me.

@bircher, I would really appreciate your inputs on this!

Could you, please, add a test case to prove this?

Tried the patch locally, but it results in a fatal error:

 [error]  Error: Call to a member function listAll() on array in Drupal\config_ignore\EventSubscriber\ConfigIgnoreEventSubscriber->getIgnoredConfigs() (line 200 of /vagrant/htdocs/web/modules/contrib/config_ignore/src/EventSubscriber/ConfigIgnoreEventSubscriber.php) #0 /vagrant/htdocs/web/modules/contrib/config_ignore/src/EventSubscriber/ConfigIgnoreEventSubscriber.php(109): Drupal\config_ignore\EventSubscriber\ConfigIgnoreEventSubscriber->getIgnoredConfigs(Object(Drupal\Core\Config\DatabaseStorage), Object(Drupal\Core\Config\CachedStorage))
#1 /vagrant/htdocs/web/modules/contrib/config_ignore/src/EventSubscriber/ConfigIgnoreEventSubscriber.php(84): Drupal\config_ignore\EventSubscriber\ConfigIgnoreEventSubscriber->transformStorage(Object(Drupal\Core\Config\DatabaseStorage), Object(Drupal\Core\Config\CachedStorage))
#2 [internal function]: Drupal\config_ignore\EventSubscriber\ConfigIgnoreEventSubscriber->onImportTransform(Object(Drupal\Core\Config\StorageTransformEvent), 'config.transfor...', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#3 /vagrant/htdocs/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(111): call_user_func(Array, Object(Drupal\Core\Config\StorageTransformEvent), 'config.transfor...', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#4 /vagrant/htdocs/web/core/lib/Drupal/Core/Config/ImportStorageTransformer.php(120): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('config.transfor...', Object(Drupal\Core\Config\StorageTransformEvent))
#5 /vagrant/htdocs/vendor/drush/drush/src/Drupal/Commands/config/ConfigImportCommands.php(262): Drupal\Core\Config\ImportStorageTransformer->transform(Object(Drupal\Core\Config\FileStorage))
#6 /vagrant/htdocs/vendor/drush/drush/src/Commands/config/ConfigImportCommands.php(47): Drush\Drupal\Commands\config\ConfigImportCommands->import(NULL, Array)
#7 [internal function]: Drush\Commands\config\ConfigImportCommands->import(NULL, Array)
#8 /vagrant/htdocs/vendor/consolidation/annotated-command/src/CommandProcessor.php(257): call_user_func_array(Array, Array)
#9 /vagrant/htdocs/vendor/consolidation/annotated-command/src/CommandProcessor.php(212): Consolidation\AnnotatedCommand\CommandProcessor->runCommandCallback(Array, Object(Consolidation\AnnotatedCommand\CommandData))
#10 /vagrant/htdocs/vendor/consolidation/annotated-command/src/CommandProcessor.php(176): Consolidation\AnnotatedCommand\CommandProcessor->validateRunAndAlter(Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
#11 /vagrant/htdocs/vendor/consolidation/annotated-command/src/AnnotatedCommand.php(302): Consolidation\AnnotatedCommand\CommandProcessor->process(Object(Symfony\Component\Console\Output\ConsoleOutput), Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
#12 /vagrant/htdocs/vendor/symfony/console/Command/Command.php(255): Consolidation\AnnotatedCommand\AnnotatedCommand->execute(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#13 /vagrant/htdocs/vendor/symfony/console/Application.php(1000): Symfony\Component\Console\Command\Command->run(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#14 /vagrant/htdocs/vendor/symfony/console/Application.php(255): Symfony\Component\Console\Application->doRunCommand(Object(Consolidation\AnnotatedCommand\AnnotatedCommand), Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#15 /vagrant/htdocs/vendor/symfony/console/Application.php(148): Symfony\Component\Console\Application->doRun(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#16 /vagrant/htdocs/vendor/drush/drush/src/Runtime/Runtime.php(118): Symfony\Component\Console\Application->run(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#17 /vagrant/htdocs/vendor/drush/drush/src/Runtime/Runtime.php(49): Drush\Runtime\Runtime->doRun(Array, Object(Symfony\Component\Console\Output\ConsoleOutput))
#18 /vagrant/htdocs/vendor/drush/drush/drush.php(72): Drush\Runtime\Runtime->run(Array)
#19 /vagrant/htdocs/vendor/drush/drush/drush(4): require('/vagrant/htdocs...')
#20 {main}. 

+++ b/src/EventSubscriber/ConfigIgnoreEventSubscriber.php
@@ -194,7 +197,7 @@ class ConfigIgnoreEventSubscriber implements EventSubscriberInterface {
+    foreach ($config_list->listAll() as $config_name) {

The listAll method is not possible, since $config_list is an array.

RE: #8, I'm sorry for the mistaken patch. Here's the Updated Patch. Tested manually. I'll add test case later on.

You have the right idea, but I think it is the wrong approach.

I think the logic needs to be re-ordered.

1. assemble patterns and save as a property so as not to invoke the hook when just accessing the patterns.
2. loop over collections (of the transformation storage) {
3. get ignored config names/keys given the destination storage
4. for cases without a key, read from the destination storage and set it in the transformation storage
5. for partially ignored config merge data and write it to transformation storage. (if transformation storage is empty use destination wholesale)
6. get ignored config names/keys given the transformation storage
7. array_diff_key transformation only config names and remove from transformation storage (or not, to be defined/configured)
8. } end of loop, end or method

This means that if there are collections that don't exist in the transformation storage they will be removed by the import/export. We can address collections in a second step easily in #2973431: Ignore configuration storage collections

RE #12 I had not reviewed all patches in the previous issue.
I just checked the patch you linked to and the algorithm you used for the transformation looks good (and similar to what I commented in #11.) I don't know why it got lost in comment 34 on that issue.

That said; the patch you linked to also has problems, in particular it uses $this->configFactory, it ignores collections and the top level loop is not efficient. So we can not just go back and use that patch as is. But I would like to encourage you to adapt the current code with the algorithm you devised or the one I alluded to in #11.

Taking a look

I believe this test asserts that ignored config entities are incorrectly deleted.

I tried to wrap my head around the logic in #12 but ran out of time.

Could we provide a test with ConfigWithTranslationTest, which uses Drush to perform the import/export? I really don't trust ConfigIgnoreBrowserTestBase::doExport(), which seems to ignore the scenario where a config already exists in sync storage but is missed from the active storage. In that case, ::doExport() should delete the sync config. But I don't see any line of code where this happens.

RE #17: Thanks a lot for taking a look and thanks for the patch.
I think implementing #11 is the most straight forward at this point as the patch linked in #12 has a lot of noise in comparison.

RE: #18: I just checked and yes \Drupal\Tests\config_ignore\Functional\ConfigIgnoreBrowserTestBase::doExport is very wrong.

it should be:

  use \Drupal\Core\Config\StorageCopyTrait;
  
  /**
   * Perform a config export to sync. folder.
   */
  public function doExport() {
    // Setup a config sync. dir with a, more or less,  know set of config
    // entities. This is a full blown export of yaml files, written to the disk.
    // We could also just use \Drupal::service('config.storage.sync');
    $destination_storage = new FileStorage(Settings::get('config_sync_directory'));
    // Importantly export form the export storage so that the export transformation is triggered.
    /** @var \Drupal\Core\Config\StorageInterface $source_storage */
    $source_storage = \Drupal::service('config.storage.export');
    $this->copyConfig($source_storage, $destination_storage);
  }

I am pretty sure though that the patch from #15 would still fail and demonstrate the bug here.

Looks like the changes to doExport() do expose some additional test fails.

• Renaming issue, I think this summarizes the problem.
• Removed tests tag as they were added in #3117694-15: Make all available tests pass
• I found it a bit difficult to understand fully what is requested in #3117694-11: Make all available tests pass, hopefully my changes make sense.
• Added a static cache for ignore and exception rules.
• Added invalidation + tests for static cache.
• Fixed core_version_requirement for test module (fails from above).
• Minor code cleanup to lines changed in patches from this issue.
• The test from 15 is now passing
• Test only patch is the same as patch, except it does not get ignored for destination, per HEAD (within transformStorage)

(edit: ignore the first 3117694-active-config.patch file)

• Fixed bad covers.
• I realized normally the events arn't dispatched again if object is re-used, since result is statically cached by \Drupal\Core\Config\ManagedStorage::$manager. Changed test to clone objects without this primed variable before simulating an export.

I found unexpected behaviour when manually testing this with the following steps.

1. Composer update to drupal/config_ignore:3.x-dev and apply patch from #25
2. Added 'system.site.*' to ignored settings and saved
3. Exported configuration and confirmed that was added to config_ignore.settings.yml
4. Modified the site name
5. Export configuration
6. Expected behaviour: site name in system.site.yml would not be exported
7. Actual behaviour: site name in system.site.yml was exported

My mistake in #26. That won't match system.site:name When I fix that. the export/import works perfectly. Thanks @dpi!

I am not sure it is correct though.

I understand my previous comments about the algorithm is a bit cryptic. I posted from my phone so I try to explain it again with more detail:

point 1 is addressed very nicely in the last patch.

in the loop we do:
For all ignored config in the destination storage we read it from the destination and set it in the transformation. This is the essence of ignoring config changes. For partially ignored config we merge it smartly, in particular if the transformation storage has it empty we use all of the destination.

Then for all of the ignored config (partially or completely) that exists only in the transformation storage but not in the destination (ie the config in sync but not active when importing or active but not in sync when exporting) we need to decide/configure what to do. (point 6&7 from #11)

Currently in 2.x we do nothing with this, which means that a new webform in the sync directory will be created but then subsequently ignored (assuming you wholesale ignore all webforms) or new blocks will be placed but then they can be changed in prod and subsequently ignored.
But I guess the expectation when exporting is that webforms added in prod are not exported. I can see the case for both so I would make it configurable and leave it to the user. But if everyone here prefers always ignoring I can live with that too.

I just thought it would be clearer to understand what is going on when looking at the code rather than merging the config names from transformation and destination and then deciding what to do depending on if there is data in the storage or not. In particular because if both lists are separate then we know which storage has the data.

We need test for what I described here: #2857247-21: Support for export filtering via Drush We do have this feature now but I still think there is not a great deal of consensus of what should happen and there is no very obvious test that covers all cases next to each other.

+++ b/src/EventSubscriber/ConfigIgnoreEventSubscriber.php
@@ -176,26 +198,12 @@ class ConfigIgnoreEventSubscriber implements EventSubscriberInterface {
+    [$patterns, $exceptions] = $this->getRules();

Sorry for being a pedant, but an array return is a bit of a code-smell. Are we missing a value-object here?

Just adding a feedback that #25 works for me.

Moved to RTBC by mistake. Moving back to Needs work.

But reinforce that it works in my use cases.

+1ing that #25 fixes the issues for us as well.

Re #28 I think this is still quite cryptic (at least for me) and maybe we just need a use-case that is currently not covered by #25, even better if we get a failing test for it.

Attached is a patch that implements the algorithm I was talking about and which fixes a bug with config that exists only in certain collections.

It still needs more tests, but maybe we add them when adding different behaviors (like don't ignore the first time it is imported, but always ignore on export etc)

Thanks @bircher. I will test this out with one of our sites.

I just did #3186392: Port IgnoreKernelTest from 2.x to 3.x locally and found two more bugs in the patch I wrote. So I will commit that first and then the fixes here.

RE #32 the issue mentioned in the previous paragraph has tests for a few edge cases, and to be honest those kernel tests I added to 2.x is really the only tests I care about.

I also re-tested the patch from #25 just for comparison, if fails locally the same tests my patch does so in that respect the two are fairly equivalent.

Attached is a patch that doesn't fail the tests from 2.x (which are now merged)

interestingly the test failures are also related to #3117809: TypeError: Argument 1 passed to NestedArray::unsetValue() must be of the type array, null given and so that issue should be resolved with this too.

Adding credits from #3117809: TypeError: Argument 1 passed to NestedArray::unsetValue() must be of the type array, null given and #3203587: TypeError: Argument 1 passed to Drupal\Component\Utility\NestedArray::unsetValue() must be of the type array, bool given

a month without complaints, I am committing this now. Thanks everyone for the help!
I will tag a beta release now. Then next up is #3223748: Allow to configure different behaviors. and we make a stable release.

Nice work!

Awesome work, tested the beta and it seems to work nicely!