Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Add a new method to the DrupalConfig class that allows you to revert to the default provided by the module. This basically replaces variable_del from Drupal 7 where variable_del was being used to revert back to the default provided by a variable_get.
Sample usage:
$config = config('aggregator.admin');
$config->set('aggregator_fetcher', 'test');
$config->save();
$config->revert('aggregator_fetcher');
$config->save();
This patch requires that a config object / file is named module_name.config_space.xml so that we can work out which module is providing the defaults.
Comment | File | Size | Author |
---|---|---|---|
#46 | interdiff_45-46.txt | 1.18 KB | ranjith_kumar_k_u |
#46 | 1497268-46.patch | 15.3 KB | ranjith_kumar_k_u |
#45 | interdiff-44-45.txt | 9.27 KB | thenchev |
#45 | configuration_revert_1497268_45.patch | 15.3 KB | thenchev |
#44 | interdiff-42-44.txt | 6.84 KB | thenchev |
Comments
Comment #1
alexpottPatch now has tests and fixes a php warning it was causing.
This approach might not correct as it introduces a dependency on the name of the config file and module. The better option is probably to scan the directory for the file.
Comment #2
sunComment #3
vlad.ilyich CreditAttribution: vlad.ilyich commentedpossibly stupid comment - don't we need to be mindful of the fact that not all default configuration at module install time is not convered by what's in the file? any module that checks the state of say, installed views, or content types, or roles, or image styles, or ..., and provides a different install time default based on that?
checking files to revert to a default will surely cover some cases. perhaps we need to fire some code here?
Comment #4
tim.plunkettI'd think this would be better served using whatever comes out of #1515312: Add snapshots of last loaded config for tracking whether config has changed
Comment #5
sunI may be mistaken, but I think that the Config class is the wrong place to add a revert method.
We already discussed the sense and nonsense of a revert functionality in:
#1398040: Detect if default configuration of a module has been changed, and allow to restore to the original
So I think that, if at all, then a revert method belongs onto ConfigEntityBase. (which sorta ties into #1642062: Add TempStore for persistent, limited-term storage of non-cache data instead of reverting to default config)
Comment #6
jibranas per #4
Comment #7
gddI talked to timplunkett some time back when we were working on #1515312: Add snapshots of last loaded config for tracking whether config has changed. So one thing is that not everything can be reverted. For instance many field-related changes can not be undone (they change data irrevocably.) This means in order for revert to work at all, we need to add the concept of things that can and can't be reverted. We both agree with sun that this should be added on ConfigEntityBase, then the individual implementations can decide whether to use it or not. This would be similar in approach to #1826602: Allow all configuration entities to be enabled/disabled. I know he planned to work on this for Views so I'll check in with him to see what his status is.
Comment #8
tim.plunkettI'm pretty sure dawehner had code for this in a sandbox, I'm not sure how functional it was. But yes, what @heyrocker said is accurate.
Comment #9
dawehnerI know damian started on #1790398: Re introduce revert functionality for views using the config system but now I'm currently working on a generic functionality for the config entity, not specific for views.
Comment #10
dawehnerJust some basic outline what tstoeckler and I have in mind:
You don't want to couple that directly to the ConfigStorageController, as this is not what we want all the time.
What do you think about that in general? Just posting random code.
Comment #11
ianthomas_ukI have been advised that for configuration sets with variable names (e.g.
\Drupal::config('menu.entity.node.'.$content_type')
), defaults should be provided via hooks (in the case of the example by implementing menu_content_type_insert and menu_install).If this is best practise, then any patch for this issue needs to take that into account.
Comment #12
pfrenssenWe now have StorageComparer which we can use to check if something is overridden.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedthere is no DrupalConfig anymore.
if this really is for Config, then i don't think we want it.
if it's for ConfigEntities, meh, i don't have strong opinions.
Comment #14
mtiftRelated: #1790398: Re introduce revert functionality for views using the config system
Comment #15
jhodgdonIs this still on the table?
Comment #16
dawehnerI don't know to be honest. (unassign from myself)
Note: For some usecases of revert you can use config_devel, which reverts all the changes to the config files for you.
Comment #17
jhodgdonSee also https://www.drupal.org/sandbox/jhodgdon/2391835 - a sandbox module that has config revert functionality.
Comment #18
jhodgdonI was looking at this issue and other Core issue to see whether my sandbox (see previous comment) should/could be added to Core to resolve them. But the others, and I think this one too, are really feature requests. So given where we are in the cycle, I'm afraid this is 8.1.x... "Add functionality" to me sounds like a feature request?
Comment #19
tkoleary CreditAttribution: tkoleary at Acquia commentedAdded to config management usability meta #2642404
Comment #20
tkoleary CreditAttribution: tkoleary at Acquia commentedAdded to config management usability meta #2642404: [meta] Usability improvements to configuration management post 8.0
Comment #21
jhodgdonBy the way, if anyone wants to pursue this issue further... the (currently contrib) Config Update project has some code to manage diffs and revisions in it:
https://www.drupal.org/project/config_update
It is fairly well developed, and has an 8.x full release now. There are services for diffing and reverting in the base module, which is being used by several other contrib modules like Features that are related to config management. There is also a UI module in the project that has UI for reports, diffs, and reverts.
Comment #22
xjmComment #29
mpotter CreditAttribution: mpotter at Phase2 commentedSorry to bump an old issue, but in studying different modules that "revert" or "import" config, found that Core code in ConfigInstaller also does this and potentially just needs a simple refactor to provide a common method that can be used:
Working on ConfigInstaller patch here: #3002026: Improve API for creating/updating configuration as part of CMI 2.0 initiative. Patch is still in progress, but comments welcome there.
Comment #36
thenchev CreditAttribution: thenchev at Acro Commerce commentedTime to wake up this beauty up after so long
The code is from the earlier mentioned Config Update module
This is mostly to get the ball rolling.
Is the core Config class the right place for this?
Do we consider the revert functionality to be important enough to be in the Config class or do we want to put it in a EXPERIMENTAl module first?
There is a dependency on Drush, this is probably something we don't want to include?
Comment #37
alexpott@thenchev thanks for working on this.
I think the revert functionality should either live in it's own service in core.services or maybe in the ConfigManager. I think the Config class should remain unaware of whether it is dealing with a configuration entity or (so-called) simple configuration.
With respect to the drush - once this is part of core we should open an issue to add a revert command to Drush. And we also should have an issue to move move config_update to using the new core functionality.
Comment #38
thenchev CreditAttribution: thenchev at Acro Commerce commentedThanks @alexpott
Created a service for the revert functionality, added it also to the ConfigManager. Let me know if that works.
I still need to do some more testing and upload a patch with the test coverage so this is still WIP
Comment #39
thenchev CreditAttribution: thenchev at Acro Commerce commentedUpdated dependencies
Comment #40
yogeshmpawarComment #41
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedJust checked with @thenchev on slack, and I'm going to fix the patch so that the tests run.
Comment #42
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedUnused use statements removed.
Comment #43
alexpottI don't think we should be adding revert functionality to the ConfigManager in this issue - could be a follow-up if we decide that's what we want.
I don't think these should be services. I thunk we should create the classes in the ConfigReverter constructor. Or something similar. The reason being is that this should never really be swapped.
As above - let's not make this part of the config manager just yet - these no use-case atm.
There is a unit test for this class in the config_update module - we should definitely be bringing it in as part of this change.
The arguments here are pretty interesting. I guess the arguments are from the config_update module but it feels funny to have to work out the type and the name before calling this. Also it's odd because we have special code to add it all back together again.
I would have thought here that we should supply just the full config name and then call \Drupal\Core\Config\ConfigManagerInterface::getEntityTypeIdByName() to determine if it is a config entity or not and then do the write thing.
This would allow us to remove ::getFullName() which I really don't think should be here.
I think both the return FALSE here should throw an exception that can be caught by the caller. The caller really should only be providing configuration that can be reverted. Potentially the question of revertable-ness should be answered in another method on this service.
The copy of the _core hash here gets to an interesting. It's not really revert to state at install - it's revert to the current state of the configuration provided by the module - so things might have changed. I think we should be re-calculating this in the same way that the ConfigInstaller does.
And this in turn make me realise that we have further complexities to deal with. It's quite possible that dependencies of this configuration as it is provided by the module might have been deleted and therefore the configuration might not actually be valid for the current site. This adds a whole layer of complexity that is just not dealt with by the contrib code but something we'll need to handle if we're going to do this in core.
Comment #44
thenchev CreditAttribution: thenchev at Acro Commerce commented1. Sounds good, this makes the patch a lot smaller.
2. Moving to the constructor (LocaleDefaultConfigStorage does the same thing)
3. Removed the Interface
4. WIP
5. WIP, still testing it
6. Makes sense, WIP while doing the refactor
7. Relevant issue from config_update module for handling the core hash => #2829506: Simple config doesn't retain config hash when reverted. Also you are right, if the config is completely removed from the module that provided it then there will be issues. Do we need to handle that case? I think we should throw an exception if there is nothing to revert to, even if removing it is the correct state of the config.
Comment #45
thenchev CreditAttribution: thenchev at Acro Commerce commentedAddressing 4. request for adding tests. This is an initial implementation directly from config_update
Comment #46
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedComment #49
alexpottTagging to make this part of #3274999: Distributions and Recipes initiative overview and roadmap
Comment #50
nedjoA basic problem with the current patch's approach is suggested in this open issue on Configuration Update Manager: #2852638: RevertInterface::revert() and ::import() do not handle config dependencies. See also this prior issue: #2423211: Review config dependency handling in import operation.
Configuration Update Manager is a great module and super useful for many purposes.
Still, the root problem is that Configuration Update Manager methods act on individual config entities, treating them as if they were atomic entities. Existing core config handling in
ConfigImporter
andConfigInstaller
, in contrast, handles configuration as sets of (often interrelated) entities, including dependency relationships.In Configuration Synchronizer, we use
ConfigImporter
to accomplish config reverting. It's not clear there's a need in core for a new specialized API method to apply only to individual config items.