Checkpoints can now be deleted 📌 Allow deleting checkpoints Active
But newly added checkpoints can not be "undone" until we have #3409718: Add CheckpointListInterface::merge() or ::squash() to allow NOT keeping a checkpoint after application →
On a related note.. I have not found the reason for this bug to occur, but maybe after all this time we can also accept the workaround and not batch the config import and just do it in one shot just like the drush command does.
After all the splits are not expected to have a huge amount of config in them when this is run from the UI.
(yes I am aware that this is an assumption that may not hold in cases where people use config split in very wild ways)
But it is better to work in most cases since it is now very easy to break it it will still be a net positive.
if you can easily go back to 1.x then maybe you have not run the update hooks?
In any case I would need a lot more information to know what is going on in order to help you.
Try to reproduce the issue with a vanilla Umami installation.
If the value is a string your split configuration is corrupted.
If you want to change the code to account for this error and silently ignore it you should add the check when the value is read from the config.
so here:
$partialSplitList = $config->get('partial_list');
if (!empty($partialSplitList)) {
----
There instead of checking if the list is not empty we can also check if it is an array.
like so:
$partialSplitList = $config->get('partial_list');
if (!is_array($partialSplitList)) {
$this->logger->error(sprintf('The partial_list config of % is not an array', $config->getName()));
}
elseif (!empty($partialSplitList)) {
....
(of course the above code means injecting a logger., I don't know why we don't have one already...)
Aha! So this is a very important detail indeed. Maybe I need to write it somewhere more explicitly, but when you split the splits things get funny. Because the event subscriber takes all splits that are available in the storage and merges their configs in, but if you split splits then those splits only become available by virtue of the previous split having been merged. So they don't get processed.
Fortunately there is an easy way around this: set a different priority for the first split (ie the one containing the other splits).
In settings.php do this:
$settings['config_split_priorities'] = [
// If you look at the code where this is used you see that the priority is the export priority and the import priority is the negative value of it. so to give it a higher priority on import you give it a higher "weight".
'parent' => -10,
];And for those who split off the config split module itself... well, you need to make sure that the module is installed before the config import otherwise none of the event subscribers are registered and there is nothing I can do about it.
I was not able to reproduce this bug. could you please elaborate on how to reproduce it?
This is maybe due to a misunderstanding. When ignoring config on import the module uses the configuration set in the sync storage.
see 🐛 Use ignore settings of the target storage if the config ignore config is ignored Needs work or ✨ Account for Config Overrides of config_ignore.settings Active .
I understand that this can be surprising so maybe it is worth addressing.
I set it to "maintainer needs more info" to see if I understood correctly.
I think this is a misunderstanding of what "ignore" means.
When you ignore something it will not be removed or "unset", this is the same for whole config entities/objects or keys therein.
When something is ignored it means that it will be left unchanged. So if it is there it will remain the same, if it was missing it will not be added. So if you look at what the code is doing, it is preparing the transformation storage so that when the destination is replaced by the transformation storage it will have that effect. So when something is ignored and it exists in the destination it will be used to set it in the transformation and if it doesn't exist in the destination it will be unset in the transformation.
If you want a different behaviour then you will have to create your own config transformation.
I set this to "maintainer needs more info" in case I misunderstood this issue. otherwise I will close it eventually.
I am unsure how to help you here.
If neither of the storages have any data then clearly your event subscriber needs to have different priorities. Config Ignore can only ignore things it knows about, so if you want to use it you have to make sure it runs after the other transformations.
However, I think maybe it is worth merging always (?) the active config (including config overrides) with the ones from the transformation storage. Or maybe just on a config import.. I think that this is one of the things people get confused by most. Maybe configurable with another $settings entry for power users.
thanks for reporting this bug
Hi fago
I have reviewed the issue you linked and the project you had this problem with.
It is a very interesting project, so I am inclined to help you out.
But I came to the conclusion that you are bypassing the import and export config transformations.
Your module transfers versions of the active config between different environments. The "big" CMI2 innovation from Drupal 8.8 was that we have the import and export transformations that happen to a storage before respective after the config storage is passed between environments. Traditionally that is with git and the ../config/sync directory that the sync storage writes to. In your module you invent a new transport layer for the "sync storage". But you don't do the import and export transformations (
by using the export storage and the import storage transformer services →
) so actually you add a transport for a version of the active storage.
So in a way you are adding another paradigm and I don't understand why. So I am not inclined to support that with Config Ignore.
If you want a visual illustration check page 15 of my devdays 2019 slides. You can replace the active storage with a snapshot and the sync storage with the transport layer you invented.
bircher → made their first commit to this issue’s fork.
Ok, Well I guess this issue is for config ignore 2.x anyway which is no longer supported.. so we don't even have to add any conflict .
Thank you for reporting this issue.
I think the easiest way forward is to use config split 2. and config ignore 3.x and uninstall config filter.
Thanks for the contribution and sorry for merging this only after a year :D
Thanks for the contribution, I fixed this in the related MR from 🐛 PHPunit tests are failing on Drupal 11 Active
Thanks for the contribution, I fixed this in the related MR from 🐛 PHPunit tests are failing on Drupal 11 Active
Thanks for the contribution
That sounds like a critical bug!
After all this is the fundamental feature of this module. Given that this used to work and that Config Split hasn't changed in a while, I am tempted to think that there is a related change in Drupal core. I have run into some config validation issues with another module I am working on, but I didn't yet have time to get to the bottom of it. I have some hunches of what goes wrong though so I will look into it soon.
kristen pol → credited bircher → .
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)
- export config
- change what splits are active (in settings.php for 1.x and 2.x , or via state override in 2.)
- 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)
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.
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)
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 → .
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 → .
hestenet → credited bircher → .
hestenet → credited bircher → .
hestenet → credited bircher → .