🇨🇭Switzerland @bircher

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!

@longwave drush uses the post_update registry (ie \Drupal\Core\Update\UpdateRegistry) so the suggestion in #28 will not make a difference for drush. Drush has to anyway be updated so that can override the protected $updateType property to detect the deploy hooks with it. I would consider making this easier for drush in a follow up issue. Then drush could could use the factory or whatever we add to make this a usable API.

I also created a PR for drush that should make this work regardless of what gets committed here.

I added a small change that would make Drush continue to work.

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

Hi, thanks for digging into this. The real reason is a bug in core I just filed: 🐛 Dispatch config transformation event during site install from configuration Active

The workaround for now is to import the configuration right after the drupal site install.
But even when the core issue is addressed Config Split will only get to participate in the second configuration import because it is a module that needs to be installed before it can be used. This is something I am considering addressing for a future version when config split is split into a module and a php library that is available also without being installed. But that is way off topic for now.

bircher → created an issue.

There is an argument which can be made to set it to -100 for example.

But I am also interested in what people who use these two modules in combination expect to happen on export.

This seems to be the same issue as 🐛 Splits: Ensure config_ignore works for everyone - set default to functional value Needs review

And how do you expect this to work when you export the config?

If you find references in the documentation feel most welcome to update it! Everyone with a drupal.org account can edit the documentation.

This was already addressed on 24 August 2017 in #2865280: Find a better name for Graylist → where we found better names and again on 24 and 25 August 2020 in #3149562: Remove legacy terminology from UI → where the legacy names were removed form the UI.
The 2.x branch doesn't have the words you are offended by in the configuration or codebase. But in 1.x it would be a BC break so they remain. If you have an issue with that I would strongly recommend upgrading to 2.x.
This week the numbers are 37,287 vs 26,547 → (1.x vs 2.x) so you can contribute to making 1.x obsolete by upgrading your sites.

I am going to leave this issue open so that I don't have to periodically close them when people don't find the old ones.

It would be great to have more information about this.
You should see the difference in the config import UI too or by asking drush to provide it.

My bet is that it has to do with sorting. ie a bit is removed and then added again but in a different position. This is a problem only with patching of course so your workaround will get away with it because it will split the whole config.

yea we can use this issue for fixing the drush integration also going forward since in Drush 13 deprecated behaviour will be removed.
Drush 11 and Drupal 9 are EOL so we can just go ahead and do this now.
See also 📌 Remove Drush 8 integration Needs review

I don't think it is that simple.

Check out https://www.drush.org/12.x/commands/

We will need to change the drush commands a bit.

Hi thanks for trying to run tests of Config Ignore.

For the best success I would recommend using ddev and the ddev-drupal-contrib plugin.

You can read about it in the last section of the blog post on nuvole.org

This will give you a similar experience as the gitlab CI has.

And as to the issue: There is nothing deprecated about ConfigStorageTestTrait.

And the other one is not used as far as I can tell:

https://git.drupalcode.org/search?search=EntityReferenceTestTrait&nav_so...

So I am closing this issue.

And to reiterrate on #235 which was cross posted: If you already have the permission to see the form you should know that the values you are editing are not the ones used (ie the original point of this issue). Then knowing what the value is is a nice feature, but it is very context specific of whether or not that is a good idea.
If we do this in a follow up we can discuss things like adding a config schema for sensitive values or something creative like that without derailing the great progress of this issue.

Given that one of the main reasons for using configuration overrides (other than languages) is to set production only values such as API keys, I think we should be very careful disclosing the values.
Also the fact that values are overwritten is useful for people who should not have the permission to see the value, maybe even more so. So the permission as it is makes me a bit uneasy because I think it the risk may not be obvious to people giving this new permission to roles.
Ie in the current implementation you need this permission to use this cool new feature, so first reaction is to assign it and then API keys are printed to the frontend as mentioned in #234

We discussed this on slack with @alexpott:

alexpott: I think we should remove the value the disclosure and not have a permission :slightly_smiling_face:
alexpott: And then add a permission for value disclosure in a followup
alexpott: Because then what you are opting in for is way more obvious.

Thanks for checking it.

I don't know if listing commands for updating a module belong to the project page if nothing changes from the normal workflow.
When you update any module or drupal core you need to follow this procedure.
To point out that this is not new I usually reference the session at Drupal dev days in Sevillie 2017. https://nuvole.org/blog/2017/mar/30/our-presentations-drupal-dev-days-se...

The steps for updating is on page 41. I think is is also somewhere in the documentation.

oh wow, from a few seconds to minutes is quite bad. I don't know off the top of my head if the patch creation could be that significantly optimised.

I would assume that it is always going to be slower than not patching. So we could add a message to the checkbox saying that for large splits this makes a difference.

I updated the module page please check it and let me know if this would have saved you some frustration. The notice about config filter is also added the the release note. I copied it from config ignore.

RE #5: No the whole point of a major version change is that it allows "breaking changes". So I will not be adding the dependency on config filter. You should always require all your drupal modules explicitly (and in my opinion with explicit version numbers so that you don't run into problems with update hooks changing configuration that you then forget to export, drupal modules are not like php libraries, they have an impact on the database schema so they need to be managed with much more care).
Uninstalling Config Filter is out of scope for config split, but there is ✨ Suggest uninstalling when there is no other module depending on it. Fixed which has been part of config filter since 1.11 →
I try to keep the disruption between major versions of the module minimal, so yes if you don't disable patching then a bunch of the config files will be different, but it should behave largely the same. It depends on how you use config split of how much these patches affect you.