🇨🇭Switzerland @bircher

kristen pol → credited bircher → .

bircher → created an issue.

bircher → created an issue.

bircher → created an issue.

I don't think we need an update hook. There is currently no way to delete a checkpoint other than directly using the API. And since it makes all furter interactions impossible except for deleting all the checkpoints, anyone who would have encountered this bug would have solved it by just deleting all checkpoints.

Don't be shy! call the api manually, or just intall config_checkpoint_ui → and comment out the unset of the delete operation and observe that you can no longer create new checkpoints, then you fix core with this patch and see how everything works as expected.

@martybfly I think the problem is how you "switch to a different environment"

The reason it works with folders the way you do it is because the non active folders are not touched at all, but that could also lead to configuration you had being lost or exported to the main sync directory when it should have gone to the folder.

In Config Split 2 I introduced the difference between activating and enabling and deactivating and disabling.
Switching environments should be done by deactivating one and activating the other, and not just enabling and disabling splits.
It gets further complicated because you could do that in the following way in Config Split 1 (which still works in Config Split 2 as well)

1. export config
2. change what splits are active (in settings.php for 1.x and 2.x , or via state override in 2.)
3. import config

how collections work is that the config of the active split is the active config and when exported it goes into a collection, when imported it goes in the active one. For inactive splits the collection gets imported, and when you export it gets exported untouched.
Your problem is that you do not deactivate the previous environment, and so it never goes from the active config to the collection inside the active config store and so when you export the config the collection gets removed.

TLDR: deactivate the environment you are switching from (do not use settings.php to determine which environment is active but use state override)

bircher → created an issue.

dimitriskr → credited bircher → .

bircher → created an issue.

MR opened with fix.

bircher → created an issue.

hestenet → credited bircher → .

hestenet → credited bircher → .

hestenet → credited bircher → .

hestenet → credited bircher → .

Ok, so I went back and first fixed the test to use a real state object instead of a prophesised one. It passed nicely.
Then I expanded the test coverage to get parents, and it failed.
Then I reapplied the fix to the linear histroy and the tests passed again.

So this is now ready for review.

bircher → created an issue.

bircher → created an issue.

bircher → created an issue.

sure!

The reason there is no edit button is that the underlying api doesn't expose editing the checkpoint label. But if we have a test that asserts the functionality I see no harm in messing with the internals (ie it might break at any moment, therefore we absolutely need a test to verify that it works)

bircher → created an issue.

bircher → created an issue.

bircher → created an issue.

bircher → created an issue.

bircher → created an issue.

bircher → created an issue.

bircher → created an issue.

bircher → created an issue.

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.