🇨🇭Switzerland @bircher

I just wanted to chime in, because this has created one of the most subtle bug that I have ever seen.

So in principle I can't see anything wrong with this service. However, I experienced a pretty bad bug when I used this service in a AccessPolicy that I added to my site.
So to describe a bit more what is going on:
I created global user roles for editors with the permission to use ckeditor etc, but not to be assigned to users. Then I created corresponding group roles in my group_sites groups where editors are managed.
Then I added a AccessPolicy to grant the same permissions as the global role would have given you in the drupal scope if you have the corresponding group role in the active group.
Maybe (most likely) there is a smarter way to do this?

But, once I use the service added in this issue, Anonymous users get access to group content of other sites.

So I replaced this service with a service that uses the GroupFromDomainContextTrait and does

$this->domainNegotiator->getActiveDomain(TRUE);
return $this->getGroupFromDomain();

Not only does this return a group more reliably (when I reset the domain negotiator cache) but it doesn't suffer from the same bug.
Debugging a bit I found that the bug also disappears if I add the domain negotiation cache reset to line 69 in GroupFromDomainContextTrait. So maybe there is something going on there?

But also could we use RefinableCacheableDependencyInterface instead of CacheableMetadata that way I could pass the calculated permission directly when getting the group. If you want I can open new issues for those things. But I wanted to chime in here since this has taken me more hours than is reasonable to find out.

Hello nikolaat.
You see, the thing you say is technically true, except for the conclusion.
line 183 $destination_data = $destination_storage->read($config_name); could return false of course.
But you are ignoring that $config_name comes from line 165 foreach ($destination_storage->listAll() as $config_name)

and that reads:

  /**
   * Gets configuration object names starting with a given prefix.
   *
   * Given the following configuration objects:
   * - node.type.article
   * - node.type.page
   *
   * Passing the prefix 'node.type.' will return an array containing the above
   * names.
   *
   * @param string $prefix
   *   (optional) The prefix to search for. If omitted, all configuration object
   *   names that exist are returned.
   *
   * @return array
   *   An array containing matching configuration object names.
   */
  public function listAll($prefix = '');

So the storage first said that the config with a certain name is in it but then returns false?
How do you do that? That is maybe not a bug in config ignore, therefore, I want people like you who experience this to provide more information and that is how this issue is moving forward.

I don't know if we need an issue in core, I don't remember if I created one years ago. But maybe I didn't and yes we need one? But on the other hand do we?

And yes that is the trait, though for core we would probably rather use the one from 2.x ie https://git.drupalcode.org/project/config_filter/-/blob/8.x-2.x/tests/sr... (if you compare the two versions of the trait you can see where config filter 1.x does in the trait what config filter does in the event subscriber bridging the API)

But also if Config Filter is not involved any more then maybe it could also make sense for the modules that needed it to adopt that code. The original point was to make the transition from Config Filter to the storage transformation API easier. So getting rid of the dev dependency on config filter requires some amount of code, either by changing the namespace of the trait if it goes "as is" into core, or copying the trait to the module and renaming the namespace. Maybe the difference between the SyncFileStorage and the sync storage service is negligible when config filter is not swapping it out, and the active storage doesn't need to be a readonly storage if you know what you are doing?

You can tell I haven't made up my mind :D
Thanks for helping me figure this out!

Thanks! Yes a change record is a great start.
Though one of the things Config Filter is still used for in Config Split and Config Ignore is for testing. I created a trait that abstracts the difference between the config filter and core API so that one can write a Test for the module when it is using filter plugins and and then rewrite the module to use the transformation events and the same test will still pass. It was instrumental for the refactoring of the two mentioned modules.

Maybe we add the trait to core too and if it doesn't get in Drupal 11, maybe we create Config Filter 3.0.x with just the trait and none of the actual code. Does that sound like a plan?

I am wondering if we should have a dedicated page, maybe documentation? maybe change record?? I don't know what would be best.
Because i think it would be good to give more background information as to why this module is scheduled to go away and what to do about it.

I think you misunderstood the timeline.
Config Filter will most likely remain supported until Drupal 11 LTS becomes unsupported. So we probably have at least three years to go.
I reserve the right to change this decision and end support earlier if a change in core makes it a hassle to keep it supported. And if there is a need (and annotation based plugins survive into Drupal 12) extend the support.

bircher → created an issue.

Thanks for the issue and thanks for the patch! i was thinking about adding some config actions during Drupalcon Barcelona, but then I didn't really know what would be useful and I am not sure just adding things for the sake of adding things is the right thing to do.

I think we shouldn't add actions for themes until ✨ Multisite setup ignore theme Needs work in other words until themes actually can be split.

Then I have some reservations for the names of adding and removing from the lists, in particular how that works with plurals, for modules addModule becomes addModules but addToCompleteList would become addToCompleteLists and that seems awkward, but I don't know what would be better.

And finally for addModule, I don't know if setting the module weight to 0 is a good enough solution or if we should allow setting the weight or if we should check the installed modules and set a weight based on that if it is available.

And lastly of course it would be great to have a test that runs these actions by means of applying a test recipe.

I think the easiest would be to add some option to Wim Leers' config inspector → to surface sequences that have no orderby setting.

Then the medium hard part is to find out which are the easy ones that could just have one of the currently available options of "key" or "value".
Then the hard part is to identify new "orderby" options (like key-then-value which core.extension:module is using) or strnatcasecmp for filter formats (see #10 📌 Add orderby key to all sequences in core Active )
Maybe another one could by "property:weight" for sequence items that are sorted by weight.
Maybe we should also add "none" for sequences where the sort order in-itself is important data, which would allow us to tell other systems to leave it alone.
And after identifying more sort options we need to convert the sorting on save to a sorting by config schema

And finally we celebrate!

Note that adding a callback as a sorting option was dismissed because it would make the config schema more dependent on having a running drupal instance.

bircher → created an issue.

just saying that you experience this without also telling me how is not going to move this forward.

I would like to know how this can happen so that we can test for it and assert that we deal with it correctly.

Thanks for the logo, I think it is funny that it is similar to the one I was using in the slides of the Drupalcon Vienna presentation.

I changed the summary a bit to better reflect that one shouldn't use this module via project browser

bircher → created an issue.

The description is not accurate. One can not deploy a subset of the configuration with config split. Or I guess one technically could, but that is really not a good idea.

bircher → created an issue.

I can not reproduce this issue.
I tried with block.* and it works as expected.

We run phpcs in CI and it passes just fine.
see https://git.drupalcode.org/project/config_ignore/-/jobs/2664453

Also this particular rule makes the file less readable in this case. So the "fix" would be to ignore the rule for at least these files (and in particular the test files where the fixtures trip it)

maybe there was a similar issue, I closed all 2.x issues with the same message without reading what the issue was about and checking if it could still be an issue:

This issue is being closed because Config Ignore 2.x is deprecated.

The new 3,x version has been re-written from the ground up and works in a totally different way, yet it provides the same backwards compatible functionality. All the 2.x tests pass in 3.x and the 3.x codebase is easier to maintain.
Users of 2.x are strongly encouraged to upgrade to 3.x, as there is a very simple update path.

So likely this issue is no longer relevant. If you think that this issue still applies please open it again and target the new branch. All issues on the 2.x branch were closed in bulk.

For example #3150307-12: On config export / config page: Exception "Argument 1 passed to Drupal\Component\Utility\NestedArray::setValue() must be of the type array, bool given"" →

But without being able to reproduce the issue, I am not sure we can really do anything and be sure that it is the correct thing.

PS: patches are no longer tested, only merge requests via gitlab.

why would you assume that I would not be interested in fixing a bug?

Please make sure to include all possible information to reproduce the bug. Ideally with a test case of course, but just detailed instructions for how to reproduce this bug is already helpful in deciding what code to change to fix it.

This is a known issue, see 🐛 drush config:import and drush config:status always report the configuration is different when ignoring specific keys (config sorting) Active

hestenet → credited bircher → .

hestenet → credited bircher → .

hestenet → credited bircher → .

hestenet → credited bircher → .

hestenet → credited bircher → .

hestenet → credited bircher → .

This seems to be the same issue as: 🐛 Use ignore settings of the target storage if the config ignore config is ignored Needs work

This seems to be the same issue as: 🐛 Use ignore settings of the target storage if the config ignore config is ignored Needs work

Hi, I will get to it once I have some time for it, I am currently on vacation with no time on the laptop.

I think I will make 1.x and 2.x compatible with Drupal 11 and then create a 3.x and only support one branch going forward.

That said, RE: #14, your module does not seem to depend on config filter, so I see no reason why that would be a hold up. Do you use the test traits?

There is a case for removing the uuid and the default hash of course, but that is to create default config for a contrib module. That is has nothing to do with a config import or export between different environments of the same site. And as such it has nothing to do with config split.

Config Distro is meant as an alternative to the config import/export for maintaining distributions. I would argue that you should create a small module that does this with the config distro events.

it could be....

We would just have to sort the list like config ignore does so that it doesn't matter what order you put the strings in.

The config being split is either configured to be split or it depends on config that is configured to be split.
If your entity display depends on something you split and it can not be made not to depend on it, then it also needs to be split. Otherwise the config could not be imported when the split is not active.

Check composer.json

Config Filter is required when running tests.
If you want to run php unit tests of config split in a different environment than the default it is your responsibility to require also config filter.

Well, what you can do is cache the dry run.
But I don't know if that doesn't get you into trouble later. Because some config gets patched. But also, theoretically at least, things could change depending on the content of the config that gets changed. And of course there are also other modules that do fun stuff with config exports (I maintain also config ignore, but there are others too)

I don't know how to reproduce this issue.

Bear in mind the following which is written on the module page of config split and the release notes of 2.0.0:

Attention: 1.x depended on Config Filter, but 2.x uses the core API. If you have not explicitly required Config Filter before Composer will remove it when updating. But then you are left with a drupal module still being installed but no longer available in the code base.
In order to properly uninstall Config Filter you should explicitly require it and then uninstall it, and remove it from composer.json only in a later deployment.

If you upgrade, you MUST explicitly require config filter, so that you can properly uninstall it.

A very interesting feature request.
But I don't think we could make the whole split "locked" because the modules you split will always have their config split.
So this would be something for partial splits. But I don't know how one would configure that.

Then for the implementation we would have to be careful not to delete the locked patches and then just apply them backwards when exporting the config rather than creating them.

But more broadly what makes me unhappy with it is that this breaks the principle that we had so far which is that the content of the split storage is "ephemeral" in a sense that it can be recreated on every export. I don't know how to square that.
Maybe a new sub module that allows you to have a bunch of patches that just get applied? Then you would have to move the patches there. I don't really know how to best address this.

I checked the referenced issue and I have given it some thought. But unfortunately I don't have a solution.
The best I can think of is to use the export storage and copy that to the configured location. That will have the things split out that config split wants to split out, including to its collections inside of the export storage.

Other modules should not try to replicate the splitting config split is doing. That said config split doesn't write the config to the split forlder while splitting, but instead to a temporary storage and then writes it to the split forlder only when the drush config export command succeeds. In other words there is already a "dry run" splitting and we could just check all the temporary storages to see if the config you are looking for is there...

awesome that the patch is so tiny!

we are using the patch from the MR in production

bircher → created an issue.

bircher → created an issue.

bircher → created an issue.

The patch from #13 does not do what I asked in #9 and it doesn't have tests.
Without tests, the issue will have to remain "needs work".

Also please if you are considering contributing to this issue, do not manipulate the ignored settings without a ConfigIgnoreConfig because doing so will inevitably be wrong.

If you patch drupal core with 🐛 Dispatch config transformation event during site install from configuration Active

and you add the following to your settings.php it will work as expected:

// Make config split available before it is installed.
$class_loader->addPsr4('Drupal\\config_split\\', [ __DIR__ . '/../../modules/contrib/config_split/src']);
$settings['container_yamls'][] = DRUPAL_ROOT . '/modules/contrib/config_split/config_split.services.yml';

Sure, we can add a config filter plugin cache clear to the update hook as we did for config ignore. But bugs in a major version upgrade that go away when you clear caches seem very minor to me. If it prevented you from clearing caches then that would be another story, but I have not experienced this problem once so I am sure that there is a way to make the upgrade work with just the standard workflow and drush deploy.

we should definitely have a test for that! It would help me step through the code and find more edge cases.

The test does still not test what this module is made to do.

Thank you all!

thanks for reporting back

could you please update the issue summary with how you got this error?
We should make sure this is surfaced in automated tests.

I added a check to see what the mode is to make sure the update fails if you do something silly.

I see, you run tests of your site with the core test run script.
While I would strongly suggest not to do that and set up your tests with DTT, I understand that you may have a legacy setup you need to work with, I don't want to stand in your way. But do yourself a favour and start migrating to DTT and just plain phpunit tests for things that don't need a running Drupal site.

Thanks for looking into this and closing the issue.

Yes for the export it will only work with splits that use the collection storage. Otherwise config ignore has no way of knowing where the config went.

thanks for the contribution

thanks

this needs to be rebased.

Tests pass fine on dupal ci without this.
The group annotation doesn't hurt, but I also don't see any added value of having it.

This should be rebased.
But more importantly the test added in this issue is not doing its job. The test needs to assert that blocks are indeed ignored.

No I think it can still happen. The problem is that the config needs to be sorted and this is currently an unsolved core issue. So I would keep this issue open so that people find it and don't open a new one.

I moved the return statement so that we don't have to check for the default collection. And I removed the check for the direction because exporting will never have it empty unless some other event subsciber empties it and in that case I guess it would be better also to leave it empty.

Thanks for the contribution!

I set the default priority to -100 because that is a big round number which allows for plenty of numbers between.

I also set the same to the export and that will make it work for splits that use the collection storage. Splits not using the collection storage will have to manually be set to come after -100 and set to stackable. I don't think there is another way to solve this in a generic way. If this is something that someone feels strongly about I would suggest fixing that in a contrib module.

I also added the cspell file so that everything is green.

bircher → made their first commit to this issue’s fork.

alexpott → credited bircher → .

it needs a test and it also the code/implementation needs to be completely changed.

I think that this is not correct.
Because with this MR the comparer doesn't implement the interface this will break custom or contrib code which typehints the interface but then passes in a normal comparer. Also this MR does not add the deprecated tag on the interface.

I don't know of the top of my head what other interfaces have been deprecated but looking for one and following the example would be good.

hestenet → credited bircher → .

hestenet → credited bircher → .

hestenet → credited bircher → .

hestenet → credited bircher → .

hestenet → credited bircher → .

hestenet → credited bircher → .

hestenet → credited bircher → .

Ok, I don't get it.
Now you want to just list the config in an extra verbose way? basically expanding the wildcards to the active config.
Not only do I not really see the value of that, but it is all done reinventing the code to do so. This will most probably either be wrong already or will be wrong with some changes to the logic of how it actually ignores the config while importing.

From #10:

So what I think we need to do is just log what was ignored when it was ignored and then display that information.

Also it would be good to test this feature otherwise we risk breaking it in the future.

So in other words, add a key valve store to the service, log the config whenever it is actually ignored, in the form alter get the data and display it nicely.
And add a test which also enables one of the core test modules which has an import transformation and assert that the changes from it are not listed.

Thanks for the MR!
However the approach in the code is wrong. We already have a way to know if some piece of config is ignored or not, so we should re-use that.

You touched even the code that creates the ConfigIgnoreConfig object by reading from a config storage, we need to create another one and ask it if the config ignore settings are ignored for updating and if so we use that object instead of the ones currently created.

And this is quite a big change, so we definitely also need a test for it.

I see, maybe there is some special logic to check if the whole storage is empty and then not do any further checks and so on an empty storage we should not add stuff that gets ignored.

Fantastic work on the MR!
But instead of checking the sync storage we should check the transformation storage and make sure it is in the default collection. I don't see why we would need to know what is in the sync storage at that point.

Oh ok, I see.
You want to bring back the overview of what was actually ignored.

The reason why it was not ported in the first place was that it was wrong in 2.x. The same way the current MR is wrong. It is correct if you only use config ignore, but it is not correct if you use other import transformations. I have not verified it, but I would assume that the code in the MR would also tell you stuff didn't change because it was ignored due to the excluded modules feature from core. And it gets even more funky with config split in the mix.

So what I think we need to do is just log what was ignored when it was ignored and then display that information.

Also it would be good to test this feature otherwise we risk breaking it in the future.

I think this is ok now

I think --force would be ok too..

Thanks for you contribution

bircher → made their first commit to this issue’s fork.

bircher → created an issue.

bircher → created an issue.

No this wouldn't need to be configured at a environment specific settings level.
If you ignore the config ignore config it should just take the one that it is going to then ignore anyway. so if you set your site to do so then you effectively say "I am managing the config ignore settings per site and not through configuration management".

I am of course very happy to add links to video tutorials to the project page, but I think it would be good if the information was up to date.

Hi, could you test your expectations with the split configured to use the collection storage?

If you make the config import subscriber more heavy as suggested in 🐛 Splits: Ensure config_ignore works for everyone - set default to functional value Needs review but also for export and configure the split to use the collection storage then (and use the same config ignore settings on all collection, in particular the ones of splits) then config ignore should "ignore" also the change in your split.

Thanks for your feedback.

Hi, could you please elaborate on what you did and what you expected? it is very difficult to understand what the issue is.

This is not the only issue where people report problems with the interaction of config split and config ignore. But this issue seems to have the most practical solution.
So I am curious what the expectations are for exporting the configuration. I have some ideas of what could be done but that would add another wrinkle and wouldn't be trivial and one could configure it to work that way already so I don't know if it is worth it without having some feedback from people using the two modules in conjunction.

Yes this is on purpose 🐛 Use ignore settings from storage being transformed if they exist Fixed Because otherwise you can not deploy it at the same time as a change to the config ignore settings.

However this doesn't work if you are ignoring the config ignore settings too. So there is still something we should do.
Now if we would check the settings in the target storage (ie when importing the config that would be the active site config) and if the config ignore config is ignored then we use that instead this would solve the issue for you. But then it wouldn't work any more on export as you are expecting and you would have to make config ignore not ignore the config ignore config on exporting (ie not use the simple mode). Which would mean that you can configure it to behave the way you want, but at the expense of being a bit more difficult to understand the nuanced configuration.

Just for clarification: You do ignore the config ignore config right? otherwise the first deployment would set it back and the next one would create the same problem for you.

tests seem to pass now. The test now tests #35. So I think this is ready to be reviewed again.

good point going back and fourth on the deprecation is not great.

RE #3: yes definitely needs tests! just didn't have enough time to add them.

RE #4: Yes, you are right, in the first steps there are no modules installed that could interact. However! You can add services via settings.php even from modules that are not yet installed. So we should definitely do it for both, which would mean the second import has to do much less if you add your modules services via settings.php to take effect in the first round. (which is what I think I will suggest people do for config split if they run into troubles with the non batched version or if they split their sync storage to smithereens against all recommendations)

And sorry for not having found your issue, and thanks for finding mine!