🇨🇭Switzerland @bircher

I think this looks good.
We deliberated that modules can support both versions by indirectly implementing the interface by extending the base class and then just add the new method which can then take advantage of this change. But as the documentation on the method signature states, it should also just work without the new object.

This issue is not about deprecating processItem but maybe we can have a discussion about that in a followup issue.

I had to read the issue summary and the patch (and the code around it) a couple of times to understand what is going on. And I think I understand what is happening.
But in order for future me to be able to maintain it I think it would be good to have an explicit test covering the scenario.

Because I think there is a mistake in the issue summary. And maybe there is a bug in core or core does re-evaluates the list like this patch does?

So first a correction: The entities which have their config patched are the ones calculated by manager->getConfigEntitiesToChangeOnDependencyRemoval as "update". The ones marked as "delete" are always completely split.

And just to test if core does it correctly, in your setup if you uninstall the module you split (via the UI) doesn't it also say your group roles will be deleted? and if you go ahead and do the uninstallation does it actually delete them?

I am not against your proposed change here, but I worry a bit that it creates another edge case when a config depends on another config that depends "optionally" on the module being split, but *also* directly on the module. But maybe I am overthinking it.

The part that bothers me though is that getConfigEntitiesToChangeOnDependencyRemoval is supposed to do the heavy lifting. It loops and updates the dependency graph etc. So if that list is not correct then I am inclined to say it is a core bug.
The idea with the patches is that you can split off some config and the result is as if the module was not installed. And so to get there we have to simulate the module being uninstalled (without it actually being uninstalled). So if core returns your group role to be deleted, but then doesn't delete it because it re-calculates the dependencies when doing the uninstallation then I guess we also have to re-evaluate.

Does that make sense?

Hello

Thanks for caring about coding standards. But phpcs is already happy.
Look https://git.drupalcode.org/project/config_split/-/jobs/4330323

I am pretty sure this problem stems from the fact that your split configuration is corrupted.
I am all in favor of more defensive coding and we should make sure that the the inputs are all good. But the method you are patching is internal and it is better if it complains when it is fed invalid data. because most probably the other code that assumes the list is an array is also broken and may hide other bugs.

greggles → credited bircher → .

bircher → created an issue.

Attached is a patch based on the branch in ✨ Programmatically add OperationType Active maybe we should review and merge that first and then I can make a MR with this patch.

wouters_f → credited bircher → .

bircher → created an issue.

hestenet → credited bircher → .

hestenet → credited bircher → .

hestenet → credited bircher → .

hestenet → credited bircher → .

So I made a little change, and use a plugin manager to get the operation types.
This change demonstrates that we could replace the current logic with a real plugin manager.
But operation types are not really plugins in the traditional sense. Because the plugin class is an interface one can not simply create a plugin instance.

But when going down that route we could go a step further and use the plugin information derived from the in situ plugin manager to make some assertions. so for example instead of just asking the provider which operation type it supports we can check which interfaces it implements.

:wave:

I think the operation types the way they are now makes them a bit different than plugins.
Because if your provider is expected to implement the interface it becomes a dependency, whereas with normal plugins you can create a plugin for a module you don't otherwise depend on.

That said, I am sure we can use the discovery methods already available. and make it "like a plugin" without being a real plugin (or needing to change namespaces etc)

first to get it out of the way: I am not trying to derail this issue or question the validity of it, I think it is a good direction, but I have some concerns of how this all plays along. But maybe I am missing something. When adding a test for the hook I added a new type based on the existing ones and I observed that it seems a bit "disconnected".

In particular I think it makes sense that there is an attribute on the interface that we expect providers to implement. But with this hook we just let the ai module know that there is another operation type, we don't make sure that the altered operation types have an interface etc. Do we want to make this more consistent? ie discover operation types in modules src/OperationType/* rather than having a hook? (Having the hook would still allow renaming or removing operation types so there is still a use case for it.)

I added a test for adding an operation type.

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

So I ran the test many times in a row to confirm the issue locally, and looking at the error message I had a suspicion.
But I traced it down to see what is happening. And the reason it sometimes fails but not others is that we use $this->randomString() in the parts where this fails. The failure comes from the fact that we use ECA tokens, or more precisely tokens with a \Drupal\eca\Plugin\DataType\DataTransferObject, this in turn gets turned into a string by using Yaml::encode($values). But some random strings get changed by that other than just getting surrounded by '.
The example in the issue summary is KO"'>&b( which becomes 'KO"''>&b(' and that in turn does not contain the original.
This can happen in all the test methods that use the ECA token replacement, so I fixed it by fixing the randomString method in the base class. I ran tests 250 times and they succeeded every time. But we can use another approach too if this is deemed not to be the right approach.

Yes that makes sense to me.
The MR looks good and does what the issue describes.

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 → .