Config ignore currently depends on the config_filter module, that does the heavy lifting of hooking into the config part of Drupal and adjusting the way it imports and exports configuration.

With the release of Drupal 8.8, a new API is available that makes it much easier for modules to adjust the way config imports and exports work. This API superseeds the API that's available with the config_filter module.

If a 3.x version of config_ignore would be created with the new API, it would lose its dependency on config_filter and it would work by default with tools like Drush.

Proposed resolution

  • Create a 3.x version of the config_ignore module that is only compatible with Drupal 8.8+.
  • Make use of the new config event api, and thereby remove dependency on config_filter.

Current status

CommentFileSizeAuthor
#33 7603125-33.patch43.94 KBclaudiu.cristea
#33 3099674-33.interdiff.txt11.32 KBclaudiu.cristea
#32 3099674-32.patch43.55 KBclaudiu.cristea
#32 3099674-32.interdiff.txt24.86 KBclaudiu.cristea
#31 3099674-31.patch36.54 KBclaudiu.cristea
#31 3099674-31.interdiff.txt678 bytesclaudiu.cristea
#30 3099674-30.patch36.58 KBclaudiu.cristea
#30 3099674-30.interdiff.txt678 bytesclaudiu.cristea
#29 3099674-29.patch36.54 KBclaudiu.cristea
#29 3099674-29.interdiff.txt8.13 KBclaudiu.cristea
#27 3099674-27-interdiff.txt9.23 KBkim.pepper
#27 3099674-27.patch30.95 KBkim.pepper
#26 config_ignore-collections-3099674-26.patch28.53 KBmikran
#26 interdiff_17_26.patch2.88 KBmikran
#25 config_ignore-collections-3099674-25.patch31.5 KBmikran
#20 config_ignore-collections-3099674-20.patch31.49 KBayalon
#17 3099674-17-interdiff.txt5.89 KBkim.pepper
#17 3099674-17.patch27.38 KBkim.pepper
#9 3099674-9.patch28.62 KBclaudiu.cristea
#9 3099674-9.interdiff.txt18.21 KBclaudiu.cristea
#2 config_ignore-3099674-2.patch22.73 KBbartlangelaan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bartlangelaan created an issue. See original summary.

bartlangelaan’s picture

This is the patch that gives an idea how this could be implemented. As stated in the issue summary, it's just a proof of concept and is not according to standards yet.

jkswoods’s picture

Have tested this and it works fine with 8.8. On a related note, I'd have to agree with a 3.x branch for these changes, dropping the dependency on config_filter is a good move and makes the module easier to maintain.

JeroenT’s picture

Status: Active » Needs review
jkswoods’s picture

fnmatch has been used within the patch which is not available on non-POSIX compliant systems according to php.net https://www.php.net/manual/en/function.fnmatch.php

JeroenT’s picture

About fnmatch. This is also used in the current version of the module and is already being discussed in #3042661-7: Drupal 9 Deprecated Code Report.

jkswoods’s picture

Okay cool, based on the discussion in that issue fnmatch is a not a priority for now.

claudiu.cristea’s picture

Status: Needs review » Needs work
  1. +++ b/config_ignore.install
    @@ -9,7 +9,8 @@
     function config_ignore_update_8201() {
    -  \Drupal::getContainer()->get('module_installer')->install(['config_filter']);
    +  // Previously, config_filter was installed here. That's not needed anymore, as version 3.x doesn't require this
    +  // module anymore.
     }
    

    We shouldn't change existing update hooks. Projects may uninstall config_filter if they consider that it's not used by any other functionality.

  2. +++ b/src/EventSubscriber/ConfigIgnoreEventSubscriber.php
    @@ -0,0 +1,180 @@
    +  public static function getSubscribedEvents() {
    

    Needs {@inheritdoc} block.

  3. +++ b/src/EventSubscriber/ConfigIgnoreEventSubscriber.php
    @@ -0,0 +1,180 @@
    +    if (Settings::get('config_ignore_deactivate')) return [];
    

    Should have code block delimiters {...}.

  4. +++ b/src/EventSubscriber/ConfigIgnoreEventSubscriber.php
    @@ -0,0 +1,180 @@
    +   * The storage is transformed for importing.
    ...
    +   * The storage is transformed for importing.
    

    Should start with a verb. E.g. "Acts on config import/export transform."

  5. +++ b/src/EventSubscriber/ConfigIgnoreEventSubscriber.php
    @@ -0,0 +1,180 @@
    +    $active_storage = \Drupal::service('config.factory');
    ...
    +    $ignored = (array) \Drupal::config('config_ignore.settings')->get('ignored_config_entities');
    ...
    +    \Drupal::service('module_handler')->invokeAll('config_ignore_settings_alter', [&$ignored]);
    

    The services should be injected.

  6. +++ b/src/EventSubscriber/ConfigIgnoreEventSubscriber.php
    @@ -0,0 +1,180 @@
    +
    +      // This returns all config names from both the active storage as the storage that will be imported.
    ...
    +      // We remove all config that was marked as 'do not ignore' from these arrays here. This way, they will not be
    +      // imported and parsed as normal.
    ...
    +
    +        // When config that would normally would be imported also exists in the active config, we just replace the
    +        // config to be imported with the config that is in sync. This way, nothing will be imported. When a
    +        // $config_key is not available, we replace the complete config. When it is available, we only replace the
    +        // $config_key value.
    ...
    +          // When the config that would normally be imported does not exist in active storage, we remove it from the
    +          // config to be imported.
    ...
    +      // Config that exists in both storages should already be handled in the foreach of $import_storage_names, so
    +      // we don't have to handle it here.
    ...
    +        // Config that is in active storage, but not in the storage that will be imported, should be created on the
    ...
    +          // We only set & write the active value when it's actually available, so we don't create an empty config
    +          // object.
    ...
    +    // We do the same as in the onImportTransform, but instead of replacing values with the active storage we just
    +    // remove them.
    

    Remove the empty line. Format the comments to fit 80 chars.

  7. +++ b/src/EventSubscriber/ConfigIgnoreEventSubscriber.php
    @@ -0,0 +1,180 @@
    +  protected function getSettings() {
    

    Missed doc block.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
18.21 KB
28.62 KB
  • Fixed #8 remarks.
  • Replaced fnmatch().
  • Fixed test failures.
Rajab Natshah’s picture

+1

jkswoods’s picture

Status: Needs review » Reviewed & tested by the community

Re #9: Have tested locally and can confirm this is working fine.

ayalon’s picture

I tested it a lot and finally figured out that you need Drush 10. It does not work on Drush 9.7.1 at all because the new service is not used and therefore the hooks are not called. Not very obvious.

jkswoods’s picture

@ayalon the README.txt has been updated in the patch to mention requiring Drush 10.

 REQUIREMENTS
 ------------
-You will need the `config_filter` module to be enabled.
+You will need Drupal 8.8 or higher for this module to work. If you want to
+import and export config with Drush, you need Drush 10+.
 
 INSTALLATION
JeroenT’s picture

Status: Reviewed & tested by the community » Needs work

Patch #9 does not work in combination with configuration translation.

e.g. Create a webform and translate it in multiple languages. As ignored config I used 'webform.webform.*'.
When runnning `drush cim`the translated config is removed.

claudiu.cristea’s picture

Issue tags: +Needs tests

#14 needs regression test.

kim.pepper’s picture

Any examples of a test that uses config translation?

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
27.38 KB
5.89 KB

I thought I'd start working on this with a KernelTest, but ran into issues with a simple copy of the BrowserTest. There must be something I don't get about how to do kernel tests of config import etc.

Status: Needs review » Needs work

The last submitted patch, 17: 3099674-17.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

ayalon’s picture

The bug decribed in #14 makes this patch very dangerous for multilingual site because of potential loss of all new translations. So be careful using this patch until the bug is fixed.

ayalon’s picture

I'm not an expert of the "Configuration Management Interface" of Drupal.

But here is my proposal, how to fix the issue with the translations (collections) of ignored configurations.

The patch is based on #9.
Please give some feedback, if you think this is a possible solution.

kim.pepper’s picture

Status: Needs work » Needs review
kim.pepper’s picture

@ayalon Looks like your patch has some issues. Did you follow the steps outlined in https://www.drupal.org/node/1054616 ?

ayalon’s picture

I can apply the patch using composer patches. Not sure why you have issues.

claudiu.cristea’s picture

Status: Needs review » Needs work

@ayalon, I cannot apply it either:

$ curl https://www.drupal.org/files/issues/2020-02-19/config_ignore-collections-3099674-20.patch | git apply
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 32249  100 32249    0     0   127k      0 --:--:-- --:--:-- --:--:--  126k
error: corrupt patch at line 885
mikran’s picture

I managed to apply the patch by adding a newline to the end of the patch file.

Here is the patch with just the newline added.

mikran’s picture

#20 did not include the changes made in #17, so here is a new rerolled patch with those changes included, and an interdiff.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
30.95 KB
9.23 KB

This builds on top of #26 fixing a couple of tests an code style.

Status: Needs review » Needs work

The last submitted patch, 27: 3099674-27.patch, failed testing. View results

claudiu.cristea’s picture

Here's a test that checks the export/import but, this time, is using Drush. Unfortunately I couldn't follow the changes since #9, so this patch is built against #9. The test proves that both, import & export are buggy.

claudiu.cristea’s picture

Hm, the first test should not fail in that line. Look like there's an issue with the file permissions?

claudiu.cristea’s picture

claudiu.cristea’s picture

Here's the fix. Note that the main test fails on composer as this needs Drupal >=8.8.x, so check only the right test. Normally, this patch, if it's approved will be against a new 8.x-3.x branch.

claudiu.cristea’s picture

claudiu.cristea’s picture

FileSize
921 bytes
44.02 KB

list() shorthand not supported in 7.0

Status: Needs review » Needs work

The last submitted patch, 36: 7603125-36.patch, failed testing. View results

claudiu.cristea’s picture

As long as this new branch will require Drush 10, which requires PHP >=7.1, we will not support 7.0.

Also the config_ignore_deactivate kill-switch should not act when building the listeners list but on each listener. The reason is that the listeners list is rebuild only on container rebuild. Someone might add the kill-switch in settings.php without rebuilding the container (clearing the cache) and, as an effect, the kill-switch will not work.

kim.pepper’s picture

Thanks for pushing this forward @claudiu.cristea. Looks like you have addressed the issue in #14.

bircher’s picture

This looks very good.
I am going to commit this with the difference that we require ^8.8.

I have some concerns with the following: The expansion of the ignored wildcard keys is done against the active config for both import and export.

Also I inverted the for loops for the collections and ignore names, so that creacteCollection is in only one loop.

bircher’s picture

Title: Add compatibility with new Drupal 8.8 API » Create 3.x version using the new Drupal 8.8 API
Status: Needs review » Fixed
claudiu.cristea’s picture

Version: 8.x-2.x-dev » 8.x-3.x-dev

  • bircher committed 498063a on 8.x-3.x
    Issue #3099674 fix to use latest patch
    
bircher’s picture

Sorry, for some reason I had the wrong patch, fixed now. I guess I should have double checked again after my laptop froze.

ayalon’s picture

Thanks a lot for the fast adoption and all the worktime went into this.

claudiu.cristea’s picture

@ayalon, it was discovered that the current 3.x branch has some bugs. I've provided a fix in #3117135: TypeError: Argument 1 passed to NestedArray::unsetValue() must be of the type array, null given. Could you test 3.x with that patch applied?

Status: Fixed » Closed (fixed)

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