Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
In discussion with @bojanz we felt it would be useful to have a method to know if a piece of configuration is provided by a module and if it has not being changed by the user. This method could be used in post update functions by modules to change configuration (for example).
Steps to reproduce
Proposed resolution
Add ConfigCompartor class with method isModified
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#35 | 2656370-nr-bot.txt | 144 bytes | needs-review-queue-bot |
Issue fork drupal-2656370
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
dawehnerThis would also make the life of https://www.drupal.org/project/config_update a bit easier
Comment #4
mglamanWorking on a patch to add a utility method to ConfigManager.
Comment #5
mglamanHere is a new
isModified
method which compares the current_core.default_config_hash
against the actual configuration's value. This hash is created when the configuration is first installed, allowing us to tell if the configuration object was modified.Comment #7
mglaman+1 me, somehow deleted namespace in test when making patch. Fixed.
Comment #9
mglaman#7 turned out to be the interdiff. Here's proper patch.
Comment #10
bojanz CreditAttribution: bojanz at Centarro commented$active could return FALSE if $config_name is invalid, the code doesn't account for that.
In Commerce we ended up accepting array $config instead of $config_name instead, since the calling code usually needs to load the active config anyway (and can handle failures). We should at least do that here as well.
I also dislike the entire idea of ConfigManager, which "provides helper functions for the configuration system". That means it has no clear purpose, and a license to grow indefinitely. I've opened #2684081: Modules need a way to keep their configuration up to date to address the full use case that isModified() is needed for.
Comment #11
mtodor CreditAttribution: mtodor commentedI have modified patch with some changes and added additional tests.
So what I have changed is following:
1. I have added Exception in case that config is not found (could happen that config never existed or that it's deleted) - how that exception will be handled depends on module that uses functionality of isModified method.
2. I didn't change parameter type, because ConfigManager works with config names mainly and introducing array $config could make confusion.
3. Tests are covering following cases: when it's updated and not changed, was already covered with previous patch - I have added: when configuration never existed, when configuration is deleted, when it's updated and marked as "installed by module", this is case when module update unmodified config and mark it as default installed one.
Just to clarify things here, this method just provides information is configuration changed since last installation of module or not. How module will handle that information is up to module update implementation.
Comment #12
dawehnerJust a quick comment for now ...
IMHO we could just add this to its own helper class and avoid some BC break with this. Also, "Manager" is just an anti-pattern IMHO, as it is a lazy way to group just whatever you want into one class. Maybe having something like a "ConfigComparer" or so, would help a bit.
Nitpick: We don't use yoda notation, but rather just
$active === FALSE
. Nice strict check, even afaik not neeeded.Comment #13
jhodgdonSee also https://www.drupal.org/project/config_update
The base module has some classes that do exactly what this module is trying to do.
Comment #14
chr.fritsch1. I introduced a new class ConfigComparator and moved the isModified function into it.
2. I also changed the active check
Comment #28
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #29
Ankit.Gupta CreditAttribution: Ankit.Gupta at Dotsquares Ltd. commentedRerolled the patch #14 in drupal 10.1.x
Comment #30
geek-merlinThe last patch misses the most and is not a reroll. So nw.
Comment #31
karishmaamin CreditAttribution: karishmaamin at Specbee commentedRe-rolled patch against 10.1.x. Please review
Comment #32
smustgrave CreditAttribution: smustgrave at Mobomo commentedThank you for the patch just fyi
Commit check failures
The last patch doesn't pass commit checks, could you make sure to run
./core/scripts/dev/commit-code-check.sh
before uploading a patch to make sure there are no issues with code formatting. see https://www.drupal.org/docs/develop/development-tools/running-core-devel...Fixed issue which was around setExpectedException()
Comment #34
smustgrave CreditAttribution: smustgrave at Mobomo commentedAppears to be a random layout builder failure.
Comment #35
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #37
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedComment #39
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedComment #40
smustgrave CreditAttribution: smustgrave at Mobomo commentedSelf review.
This will need a change record for the new service.
Comment #41
joegraduateCreated a draft CR for review:
https://www.drupal.org/node/3350329
Comment #42
smustgrave CreditAttribution: smustgrave at Mobomo commentedThanks @joegraduate for the CR.
Moving to NW for the open threads on the MR 3338
Comment #43
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedComment #44
smustgrave CreditAttribution: smustgrave at Mobomo commented2 Nitpicky ones.
Comment #45
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedComment #46
smustgrave CreditAttribution: smustgrave at Mobomo commentedThreads appear to be answered.
Comment #47
quietone CreditAttribution: quietone at PreviousNext commentedComment #49
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedReview Comments addressed.
Comment #50
smustgrave CreditAttribution: smustgrave at Mobomo commentedFailure in MR seems legit to the issue.