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.
Problem/Motivation
Following discussion s about #1934152: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects with @xjm, @GaborHojtsy, @dawehner, @bircher, @ademarco, and @berdir,@fago, we need to ensure that config overrides (a) don't bleed into configuration when you save it (b) config overrides are used when they should be. This problem has highlighted that it is very easy for config overrides to bleed into configuration.
For example:
// Override name in settings.php
$config['system.site']['name'] = 'My Drupal site';
// Do something along the lines of:
$name = \Drupal::config('system.site')->get('name');
// Put the name in a form or something...
\Drupal::config('system.site')->set('name', $name)->save();
This is bad because:
- The code has no control over what is overridden - anything can be overridden in settings.php
- Potentially security sensitive information could end up unintentionally in configuration
Proposed resolution
- By default return immutable configuration objects from the config factory.
- Allow the developer to get mutable configuration objects from the config factory. Mutable objects should not have overrides.
- Use mutable object where necessary.
- Provide a trait so that forms can easy get mutable configuration objects - but also use runtime config with overrides.
- Config entities when loaded as entities (e.g., via the class's load() method) remain mutable. HEAD already has other code in place to make those override free in config entity forms and listings. From outside those forms and listings, it remains possible to get a mutable, with-overrides config entity and write code that results in override bleed, but this is a less severe problem, since overriding config entity values via settings.php is an extreme edge case: it wasn't even possible to do in D7 at all.
Remaining tasks
Agree approachWrite patch- Review
- Commit
User interface changes
None
API changes
An incomplete list:
- ConfigFactory::rename() returns the configuration factory and not a config object.
- ConfigFactory::get(), ConfigFactory::loadMultiple(), \Drupal::config(), FormBase::config() return a configuration object that can not be saved or changed.
- ConfigFactory::getEditable() introduced to get editable configuration objects
- New trait
ConfigFormBaseTrait
which provides aconfig()
that can return mutable config objects for named configuration. The configuration names are provided by implementations ofConfigFormBaseTrait::getEditableConfigNames
Beta phase evaluation
Issue priority | Critical because this is a blocker for providing a complete solution for #1934152: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects. This also has security implications. For example, if you override an API key in production there should be no danger of this being saved back to config. |
---|---|
Disruption | Disruptive for core/contributed and custom modules/themes because it will require a BC break. The \Drupal::config(), FormBase::config() will no longer return saveable objects. However the disruption is worth it because of the increased security, reduced fragility of overriding configuration and predictability of the configuration system. |
Comment | File | Size | Author |
---|---|---|---|
#116 | 2392319-config-object-immutable-116.patch | 811 bytes | vijaycs85 |
#81 | 73-81-interdiff.txt | 4.35 KB | alexpott |
#81 | 2392319.81.patch | 121.48 KB | alexpott |
#73 | 2392319.73.patch | 122.17 KB | alexpott |
#69 | 2392319.69.patch | 122.71 KB | alexpott |
Comments
Comment #1
dawehnerComment #2
dawehnerComment #3
dawehnerComment #4
Gábor HojtsyPostpoed #1934152: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects on this one as discussed.
Comment #5
alexpottFirst patch that makes configuration object returned from the factory by default immutable and ensure that minimal and standard still install.
A couple of interesting things are that StaticMenuLinkOverrides and SiteConfigureForm currently would bleed global overrides into the active config. I'm not convinced by my changes to StaticMenuLinkOverrides.
Sending to testbot to see how much breaks.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedThe proposal makes sense to me.
Didn't CMI at one time have a $override_free variable/concept? I wonder if that (or $with_overrides) would be a better parameter name, because for example, an override-free object should be used for setting #default_value as well, even though doing so does not require mutability (directly). That's a minor point though that could also be discussed/changed when the patch is further along.
Comment #7
alexpottre #6 - I did not force mutable config objects to be override free yet - I'm not sure about that.
Comment #9
ademarco CreditAttribution: ademarco commented@alexpott: would it make sense to have:
\Drupal::config('core.extension', FALSE);
wrapped by a more descriptive
\Drupal::mutableConfig('core.extension');
.We can adopt a similar approach in other places too, in this way we kind of state that configuration is immutable when accessed directly via \Drupal::config which is kind of what we want, is that correct?
Comment #10
alexpottTests should now at least run.
Comment #11
alexpottSo the approach in #5 and #10 is giving me nightmares about the amount that is going to have to change and whether or not it is really correct. The main issue here is that FormBase::config returns objects that can be changed. The patch attached to this comment swaps the logic around so the FormBase::config returns an immutable object but \Drupal::config() still returns a mutable object. It is still worth checking where #10 fails because it might surface other classes like
StaticMenuLinkOverrides
where overrides can potentially bleed into active configuration.I have not supplied an interdiff because this patch is an invert of the logic in #10 so that would be largely irrelevant.
Comment #12
dawehner.
Comment #15
Gábor HojtsyReviewed #11. Outside of code style (lack of docs, typos) I did not find any thing to complain about. The question as always is how far are we going to need to go to implement this approach.
Comment #16
alexpottSo
StaticMenuLinkOverrides::saveOverride()
is broken in HEAD because it cannot merge definitions as it says it can because the ID is double encoded. The patch attached makes this code simpler and tests it. All the other test fails should be fixed too.Comment #17
Gábor HojtsyWhy is this not based on ConfigEntityFormBase? Sounds like we do a lot of custom things here that would be given there.
Wow, good find.
Comment #18
Wim Leerss/an/a/
s/object/configuration object/
s/MUTABLE:/MUTABLE :/
s/config/configuration/ (if it fits in 80 cols)
s/config/configuration/
s/an/a/
Missing docs.
This is a "delete overrides" method, yet the docs indicate "saving". Is this correct?
So… we get the configuration WITHOUT overrides to ensure that GLOBAL OVERRIDES are saved?
That is very confusing.
s/override free/override-free/ ?
Comment #19
alexpottSo #2393073: Helper issue has uncovered everything that we'd need to change if we made the config factory return immutable objects by default (this would include \Drupal::config() too). @Gábor Hojtsy has created #2395395: TestBase lacks a config method to be used consistently in tests.
Other issues identified:
If we decide that we what to make config by default immutable then the three additional issues listed in this comment should be critical as they block the implementation.
One interesting thing discovered whilst implementing is how we deal with form alters. For example, contact_form_user_admin_settings_alter() / contact_form_user_admin_settings_submit(). These functions handle adding a checkbox to the user admin settings page to enable the personal contact form for users by default. The patch in the helper method suggests changing:
to
FormState::config() would delegate to the FormBase or ConfigFormBase's config() method - thereby ensuring that the correct override state / mutable or immutable object would be returned.
I think we need to somehow help people altering forms to do the right thing.
Comment #20
Gábor HojtsyI don't think we can box people into something in an alter hook. We can provide good examples that people will copy-paste anyway. Or make the whole config API abide the same rules of [im]mutability, etc.
Comment #21
alexpott@Gábor Hojtsy - I'm not talking about boxing people in - just providing a recommended way to access configuration in the same way as the form object.
Comment #22
alexpottdiscussed with @effulgentsia and @cilefen on the CMI call. We got a proposal that I'm going to write up and add here when I've got a moment.
Comment #23
alexpottNotes from the meeting alluded to in #21.
We discussed the following places where configuration objects are retrieved.
Looking at the above table we think that implementing a new trait
ConfigEdittableTrait
that provides aconfig()
function that returns a mutable, override free config object. Similar to the currentConfigFormBase::config()
. However in order to get it to return mutable, override free config the class that implements the trait will need to implement a method or property that returns a list of all config that should be editable. The advantages of this are:ConfigFormBase
they will have to think about which config objects they want to be able to saveComment #24
catchI see how this helps people defining the actual forms.
What does this change for the contact form alter example though?
Comment #25
daffie CreditAttribution: daffie commentedComment #26
alexpott@catch you're right - the form alter needs situation needs thinking about / discussing. With the solution outlined in #23 this is will make it hard for them to do the right thing unless they can also easily a mutable override object of their choice.
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedInitially my thought with form alter would be that the trait proposed in #23 would define its config() method and its addEditableConfig() method as public, so that following the example of #19, a form alter could do something like
$form_state->getFormObject()->addEditableConfig('contact.settings')
, at which point its later calls to$form_state->config()
would return a mutable config. However, that's flawed, because it creates a WTF dependency between different modules' alters. For example, if modules A and B both alter a form, and A does so by adding a submit handler that wants to send an email to the appropriate overridden value while B wants to provide a textfield for editing the non-overridden value, that approach would clash, because simply the act of enabling module B ends up changing who receives the email sent by module A. In summary, #23's proposal of having the config() method choose whether to return an overridden or non-overridden object based on the value of a stateful property is problematic if multiple modules can act on that state.So, rethinking this problem space a bit, here's my suggestion:
ConfigFactory::get()
that defaults to FALSE. Thereby any code has the ability to interact directly with the config factory (rather than one of the config() wrapper methods) in order to get an editable (override free) config.\Drupal::config()
wrapper as only returning an immutable, with-overrides, config (i.e., invoke the factory's get() without the 2nd param). Outside of forms, which I'll cover below, procedural code needing an editable config should be rare, and in such cases, it can call Drupal::configFactory()->get() and pass the 2nd param.ControllerBase::config()
wrapper, because same situation of rarity of needing editable config within non-form controllers.FormBase
andConfigFormBase
), a config() wrapper is frankly too ambiguous. You're in a form, and in both form building and in form submitting, sometimes you might want a with-overrides config and sometimes you might want an editable config. So I think the presence of an ambiguous config() wrapper method hurts more than it helps. So, I propose to remove it and instead haveruntimeConfig()
andeditableConfig()
wrapper methods to force explicit choice. This will break every already ported contrib module that has a form that interacts with config, which sucks, but I think forcing clarity around data integrity is worth it.FormState::config()
wrapper. Within the context of $form_state, there is 0 ambiguity that what you want is editable config. That will allow the more common case of forms wanting editable config to call$form_state->config()
rather than$this->editableConfig()
, which I think is nicer DX. Additionally, it will allow form alters to also use$form_state->config()
, thereby having consistent syntax between the usages within the form class and within the alters. Accessing with-override config would be different though: in the form class, it would be done as$this->runtimeConfig()
and in the procedural alter implementations it would be done as\Drupal::config()
. But I think that's ok, because if what you're accessing is runtime config, then even though you're in the context of a form, with respect to that config, you're not dealing with its "forminess", so there it's more important for the procedural code to be consistent with other non-form procedural code, and for the code within the form class to be unambiguous.Thoughts?
Comment #28
alexpottWhat was nice about the idea suggested in #23 is that developer declares there intent to edit a specific config file - and then we can use the same function to get any config file. What was really nice is that it guaranteed that both the
ConfigFormBase::build()
andConfigFormBase::submit()
would use the same config object for a specific config file. With the solution in #27 it possible to useruntimeConfig()
inbuild()
andeditableConfig()
in thesubmit()
.That said, I'm okay with most of the solution in proposed in #27. I'm not sure about having
FormBase::editableConfig()
. I still think we should haveeditableConfig()
as a trait andFormBase
forms that need this should implement on an individual basis since it is a special case. I like renamingFormBase::config()
toFormBase::runtimeConfig()
since this will alert developers if they are building a form that edits config they need to use ConfigFormBase.Comment #29
effulgentsia CreditAttribution: effulgentsia commentedTrue. That's pretty important.
Discussed this problem with @alexpott, and here's what we came up with. The following is a complete proposal that fully supersedes #23 and #27. The last item is the one that addresses the form alter problem.
ConfigFactory::get()
to return an immutable object. Thereby, all existing config() wrapper methods (e.g., Drupal::config(), ControllerBase::config(), FormBase::config()) also return immutable objects by default.ConfigFactory::getEditable()
so that all code has the ability to interact directly with the config factory (rather than one of the config() wrapper methods) in order to get an editable (override free) config.ConfigEdittableTrait
trait with a protected config() implementation and a protected$editableConfigs
property. MakeConfigFormBase
use this trait. Other classes can use this trait too. For any class that uses this trait, it can set $editableConfigs to the names of the config objects it wants to edit. config() will then return a mutable, override-free object for those names only, and fall back to the immutable, with-overrides object for other names. Note that because config() and $editableConfigs are protected, this is a local decision, so is free of the problem described at the beginning of #27. Per #28, this also ensures consistency between the form build and form submit: i.e., if a mutable object is used in the submit handler, then you also get an override-free object when setting #default_value in the build() method.By doing this, the alteration can use the ConfigEdittableTrait trait, and specify the configs that it's responsible for editing, and benefit from the same consistency of alterForm() and submitForm() using the same override state for any given config. And again, this is a local decision, meaning two different alter classes remain independent with respect to what
$this->config()
returns for each.Comment #30
Gábor Hojtsy#2395395: TestBase lacks a config method to be used consistently in tests landed, so now all issues that postponed this one are done :)
Comment #31
alexpottWork done on the patch. Posting to get a test run and so if anyone else wants to take this on they can...
Still to add the ability to define which configuration is editable on the trait.
Comment #36
alexpottThis should fix the test failures - still need to implement the ability to define which configuration is editable on the trait.
Comment #37
alexpottThe patch attached implements ability to define which configuration is editable on the trait and changes form alters to work in the way suggested in #29. I've implemented
\Drupal::resolve()
so we don't get\Drupal::getContainer
proliferating everywhere.Comment #39
alexpottFixed all the form alters are created #2401035: items_per_page in node.settings is no longer used to remove one.
Comment #40
alexpottRerolled now that #2401035: items_per_page in node.settings is no longer used landed.
There are a couple of @todo's in the patch - the most awful of which is:
The current theme settings form will allow overrides to bleed into configuration. And the work this patch does not protect against it. Considering this patch does not make the situation worse I think we could file a follow up for that?
Comment #41
Gábor HojtsyStarted reviewing the interdiffs, this one is for #37. Sorry if some may be fixed since then.
Not finished
Lacks docs, no newline.
This is somewhat duplicated in system.config_translation.yml but I don't see a good way to match the route <-> config list to the class <-> config list mappings that are here. Also this points out that we have some incorrect mappings there.
Wow, that is a pretty huge API (at least best practice) change there...
the checkbox / setting, etc. a word is missing from this sentence.
I agree we can open a followup and need to update patch to refer to that issue number.
Comment #42
Gábor HojtsyLooking at #39, the string translation trait changes seem cosmetic, but then I realized those are in "new" alter class code, so they would show up as new code in the patch anyway. So fine...
I think the main question based on the recent changes is whether we want to change form altering so much as introduced here.
Comment #43
tim.plunkettOne thing I haven't grokked is what will happen if you try to use old-style form alters. Will it save things wrong? Will it silently ignore changes? Will it blow up?
Comment #44
alexpott@tim.plunkett if you try and save config using
\Drupal::config()
it just won't work - you'll get an exception on theset()
. Of course you could use\Drupal::configFactory()->getEditable()
to get a mutable config object.Comment #45
effulgentsia CreditAttribution: effulgentsia commented(See also #29.4 and #41.4)
This doesn't change every form alter: only ones that do their own direct manipulation of simple config (rather than via a config entity or plugin). Core has ~40 form alters, and this change is only needed for 5 of them. (If
system_logging_settings
implemented a plugin API that dblog and syslog could use, then that would go down to 3).I agree that the change is significant for those cases, but I don't have any better ideas for how to give form alters an API for getting mutable config that ensures consistency between the build/alter and the submit: ensuring that consistency requires placing those two methods into the same object, I think.
Comment #46
effulgentsia CreditAttribution: effulgentsia commentedRight. The risk is that you do that in the submit callback, because you have to to avoid the exception on set(), but then you keep using
\Drupal::config()
for the #default_value in the alter hook itself, because you forget to change that and there's no exception thrown to remind you. Then you get the settings.php leakage into active config that this issue is trying to solve. Which the trait solves, but requires an object to be attached to.Comment #47
dawehnerJust a quick drive by review.
Note: chx planned at some point to override this configuration for mongodb.
maybe just doGet or something like that?
It is a little bit confusing that you can rename a immutable config.
I'd be great if we somehow document in the exception message what people should do instead.
Let's describe when to use it.
I really like that
more space :P
Comment #48
effulgentsia CreditAttribution: effulgentsia commentedUgh. I hate that we make it possible to get an editable config that has overrides applied. I was hoping this issue would put an end to that. I guess we need to for now because of
ConfigEntityStorage::doSave()
, but it would be great if we could find a way to remove needing that in a followup. In the meantime, how about removing the parameter, and creating a separate method likegetOverriddenAndEditable()
that is not inConfigFactoryInterface
, but instead is either not interface-defined at all, or is in some specialized interface with docs telling people to beware of it?Would it make sense to create a child issue that just does the getOriginal() conversions? AFAICT, those make sense regardless of this larger issue, and at least for me, distract in grokking the larger patch.
Comment #49
effulgentsia CreditAttribution: effulgentsia commentedAdditionally, what about splitting out FormAlterInterface and the 5 conversions to it (without the new trait though) into a child issue? Given #42, I think that could benefit from its own discussion thread, both the principle of it and details such as method names and whether the procedural hook or the alter() method should be the one that attaches the #validate and #submit. Also, the principle of an alter() and a submit() wanting access to a shared protected member is a valid principle independent of this use case, which I think would make that issue legitimately committable on its own.
Comment #50
yched CreditAttribution: yched commentedSorry to add a name nitpick here but since this introduces \Drupal::resolve() :
Reading code like :
it's not really intuitive that "resolving a class" means instantiating an object through a generic factory.
I know the underlying service is called "class resolver" (which never stroke me as obvious either), but not sure internal consistency is worth the unintuitive public API .
Comment #51
alexpott@yched happy to have suggestions - not wedded to the name :)
Comment #52
larowlanreview in progress
not sure this is even a word? overridable is a stretch, unoverridable might be a bridge too far. How about $cannotOverride instead?
Is there a reason for the _get? Our normal pattern is to prefix these kind of things with do - i.e. doGet()
Comment #53
alexpottre #49 and #50 I've broken out the form alter changes into #2402445: Implement object oriented form alters since it is hard to have a proper discussion on them with all the immutable config code that surrounds them in this patch.
Going to implement this without the form alter changes.
Comment #54
alexpott@larowlan #52.2 expediency - I was thinking just get the method out of the way :)
Comment #55
alexpottThanks everyone for all the reviews! The patch attached addresses the points below and reverts the form alter changes and does the minimal there so we can discuss the OO-ification on another issue.
#41 review
#47 review
#48 review
#52 review
Comment #57
alexpottThinking some more about #48.1 we just don't need it - config already prevents overrides from bleeding in on save - so getting the config with overrides in ConfigEntityStorage::doSave() and then calling setData() is kinda pointless.
Hopefully fixed test fails.
Comment #59
alexpottThis conflicted with #918538: Decouple cache tags from cache bins but git resolved it...
Comment #61
alexpottFixed phpunit fails.
Comment #62
effulgentsia CreditAttribution: effulgentsia commentedThanks for implementing all that feedback. Still reviewing, but:
Right, but get() -> getOriginal() changes more than just not applying overrides, it also means ignoring unsaved set()s. Which maybe is a no-op in the cases here (especially if only called on immutable config), but whether we have mutable or immutable config at the point that we call getOriginal() isn't immediately obvious.
Comment #63
effulgentsia CreditAttribution: effulgentsia commentedJust a reroll.
Comment #64
effulgentsia CreditAttribution: effulgentsia commentedMore thorough review on all of the get() -> getOriginal() conversions (excluding the ones in ConfigOverrideTest):
Why?
$this->getConfig()
returns editable and override-free config already.Why?
$config
is already editable and override-free.Since this is used in a form, shouldn't this be
\Drupal::configFactory()->getEditable('color.theme.' . $theme)->get('palette');
instead?Since this is used in a form, shouldn't this be
\Drupal::configFactory()->getEditable('locale.settings')->get('translate_english');
instead?Comment #65
effulgentsia CreditAttribution: effulgentsia commentedPartial review. Only got as far as language.module, and skipped over \Drupal\Core\Config
The sentence before this is great, but let's also add docs explaining why we're requiring explicit declaration of which config to make editable rather than doing all of them.
I don't see how this centralization helps the subclasses. From what I can see, it just makes them change their constructor signature for no benefit.
Is adding DI of the config factory within this issue's scope? FormBase has a configFactory() method, so how about just using that, and making the DI of it a separate issue?
Same question.
Do we need that @see at all? If so, should we point it to the trait instead?
Another case of factory injection as scope creep.
Let's file a followup and link to it. Would be good to explain what can bleed, cause it's not immediately obvious from quick code inspection.
Same.
Nice cleanup, but let's do it as a separate issue.
Unless the answer to this is a quick "nope, we're fine", let's open a followup (or add it to the earlier one).
(plus all the related changes) Seems out of scope.
And another out of scope DI addition.
Comment #66
alexpottPatch needed another reroll plus I've fixed the
StaticMenuLinkOverridesTest
to run (and hopefully) pass again.#64 review
#65 review
Comment #67
alexpottAdded unit tests for ImmutableConfig and ConfigFormBaseTrait - I think that means we have full test coverage. Discovered some bugs in ImmutableConfig :) yay for phpunit exception testing.
Comment #68
dawehnerJust curiuos, can we can the (im)mutability into the description?
just in general should those exceptions point to configfactory->getEditable?
I'm confused here ... on actual runtime we still rely on cf->get() so we get the overridden version, which itself though is also immutable, okay confusion cleared.
Comment #69
alexpottre #68
This patch also removes the cannotOverride property and canOverride method from the ConfigFactory after discussion with @catch in IRC. Whilst it is totally unadvisable to override core.extension since the raw config is read in several places to avoid circular dependencies we can just document this in
default.settings.php
. The override system in Drupal has always been powerful. D8 it is super powerful since you can also override config entities, installed modules - anything stored in config. And yes it will be very easy to hose your site BUT that is through editing settings.php or using the API in crazy ways.Comment #70
effulgentsia CreditAttribution: effulgentsia commentedOpened #2404245: Add a config override validator to throw an exception when settings.php or a module override something they shouldn't? for discussion on that. Not a blocker for this issue.
Comment #71
effulgentsia CreditAttribution: effulgentsia commentedBecause it's possible to fix without removing the function, and during the beta phase, removing an ancillary public function seems like something that should have its own discussion rather than sneaking into a 100k patch: #2404407: language_set_browser_drupal_langcode_mappings() is a useless wrapper, so remove it.
Comment #72
effulgentsia CreditAttribution: effulgentsia commentedThis looks like a vector for override bleed, since the $immutable parameter isn't paired with a setOverrideState(FALSE). Would it make sense to do the same here as for get()/getEditable()/doGet() and introduce loadMultiple()/loadMultipleEditable()/doLoadMultiple()?
This looks like unused complexity if the trait is only ever used by classes that extend FormBase, since that has a configFactory() method. What about making the trait declare an abstract configFactory(), making that explicitly part of the contract for using the trait? Doesn't seem like an unreasonable requirement even for a case of a non-FormBase class wanting to use the trait.
Comment #73
alexpottre #71 that issue has now landed. It is hardly sneaking in when we are discussing it on issue.
re #72
Patch rerolled.
Comment #74
Gábor HojtsyFun fact. I just found #2095289: Make getEditableConfigNames() public and use it in config_translation where @tstoeckler ventured into introducing basically the getEditableConfigNames() introduced here. It got some good reviews from @effulgentsia and others but then got forever postponed. Marking as duplicate of this one now.
Comment #75
tstoecklerI don't think that one is duplicate. getEditableConfigNames() is protected here, but we need to call it from the outside, i.e. in config translation module.
This is also missing the bubbling up of the config names from forms which contain plugin configurations, which is discussed over there in depth.
Comment #76
Gábor HojtsyI think making it public or private is a tiny change, would not consider that a big difference. Would we not need to bubble up all plugin config names though if we need them to be mutable?
Comment #77
tstoecklerI haven't followed this patch enough to be able to answer that question.
But sure if we can make that method public and add an interface as part of that issue, then that would indeed make the other issue obsolete.
Comment #78
Gábor HojtsyNo wait, I did not want to extend the scope of this issue, not by a single bit. I hoped this gets far enough towards #2095289: Make getEditableConfigNames() public and use it in config_translation that a miniscule diff does not necessitate that issue but the consumer issue could do that adjustment instead. I'll reopen #2095289: Make getEditableConfigNames() public and use it in config_translation postponed on this one. I still think there are lots of similarities and #2095289: Make getEditableConfigNames() public and use it in config_translation will need to be totally reworked if it is ever to be worked on again.
Comment #79
effulgentsia CreditAttribution: effulgentsia commentedI discussed #73.1 with alexpott, he'll post the conclusion of that. I agree with #73.2. I re-reviewed the entire patch again, and other than #73.1, here's all I found to complain about:
get() doesn't have a 2nd param.
I don't understand this comment. Why does overriding color config make no sense, and why is it up to this function to decide that?
Do we need that second sentence if we have the first?
If this is important to do, let's turn it into a @todo with an issue link.
Why does it not need to be saved? Because of cache? If so, let's say that. Also, on the last line should "of" be "or"?
Comment #80
effulgentsia CreditAttribution: effulgentsia commentedOops. Missed this in #79, but that 2nd param is no longer used, and isn't in the interface, so can be removed.
Comment #81
alexpottre #79
#80.1 - fixed
We need to bring the issue summary up-to-date with the current state of the patch. Going to work on #73.1 next.
Comment #82
alexpottHere's a summary of opinions I recently expressed to @effulgentsia.
I don't like this issue and the way we are dealing with overrides whilst administrating the site. It adds heaps of complexity both for the developer and the administrator. I don't like that an administrator does not know when configuration is overridden and they are seeing one thing a form edit field and another as a runtime effect. It's all feels extremely complex and confusing. I think the ability to manually set the override state from outside the factory is super dangerous.
If you have overrides in settings.php - you know you have them - deal with it. The project has chosen to implement the side effects and core should not have to deal with that. If you are using language overrides or some other override vector (eg domain) then edit your site in the default language and on the default domain.
One possible solution is to preserve D7's override bleeding in the administration system but disable all other overrides.
If we continue the way we are going I think I might repurpose https://www.drupal.org/node/1934152 to add a form element wrapper to the ConfigFormBaseTrait that indicates a value is overridden and what the override is.
On the other hand...
Multiple people working on a project - not everyone sees everything - so this makes it easier to manage.
Comment #83
aspilicious CreditAttribution: aspilicious commentedMultiple people working on a project - not everyone sees everything - so this makes it easier to manage.
Certainly when a project gets moved from one person/team to another. For example when it shifts from development to maintenance.
Comment #84
effulgentsia CreditAttribution: effulgentsia commentedI agree with #83.
Re #82, all good points. But it seems to me like when weighing the pros and cons, you're not suggesting that we change course on this issue. If I'm wrong about that, please reset this to Needs Review, but otherwise:
Setting to needs work for both of those plus the change record. More eyes on the patch wouldn't hurt, but I'm already comfortable with RTBCing this once those remaining items are done.
+1
Comment #85
alexpottLast night I was coding in my dreams and I realised we have a problem with config events and overrides - the config factory at this point with be override free for saves and deletes but not renames - so if the event wants to do something with runtime config then it has to set the config factory back to using overrides and set it back to override free. This is horrible.
Comment #86
alexpottCreated #2406543: Remove ConfigFactory::setOverrideState and ConfigFactory::getOverrideState() for #84
Comment #87
Gábor HojtsyI think we have rich experiences in Drupal 7 about override problems. We had them in Drupal 7 and we wanted to shoot to avoid them in Drupal 8. We needed to build in overrides as a core thing in Drupal 8 because we did not have a better way to implement translations, dev/prod environment specific settings, spaces, groups, domains, etc. And we needed this in core so that contrib knows that they may be there and they need to intelligently deal with it. That in itself does not predestinate the API complexity needed of course.
What we did on the API level is overrides are treated separately in the config objects so a config load, config set and config save will NOT bleed overrides to the configuration file. The realization that lead to this issue is forms, REST API endpoints, etc. are not immune to that same problem because we possibly load the data with overrides and then when we get the data back we write the original data over.
We have/had admin upcasting without overrides and the config() method in ConfigFormBase load without overrides to try to avoid that but on the course of this issue Alex found several problem areas where we did not apply our principles right in core. So the whole idea of strict immutability would be to force people to do the right thing instead of letting them do the natural thing which would likely lead to override bleeding. The proposal of this issue is that we need to declare when we get the config object that we want to write it AND we need to consistently do that in the provider (eg. form) and the updater (eg. submit). Those then would not have overrides so we don't run into this problem.
As for how common maintaining overrides separate from admin forms where admin forms let you edit the original values here is a video snippet from strongarm: https://www.youtube.com/watch?v=007YfTbiHHE#t=334. The module page says this module is used on 120k Drupal 7 sites. I am not sure telling site admins to "go to the original language" version of the config form, because that may still be on the wrong domain, in the wrong group, etc. There is a matrix of overrides by different conditions that may apply. So the forms and admin listings need to not use the overrides. If they would use them then the bleeding of sensitive data like API keys would be inevitable. We can tell site owners we told you so, but if they got used to the ways strongarm supported them to work with (see above video), that was exactly the protection we are looking at here for overrides to be able to achieve. So if that would not happen with admin lists and forms, that would be a step back. I guess a strongarm like module could still override the listing classes and the upcasting, but doing that consistently sounds like a nightmare.
Comment #88
alexpottSo re #72.1 let's just remove the ability to get multiple config objects to save. We only have one use case and it's incorrect! See #2407093: Refactor ConfigurableLanguageManager::updateLockedLanguageWeights() to save configuration entities rather than raw config
I've included the patch on #2407093: Refactor ConfigurableLanguageManager::updateLockedLanguageWeights() to save configuration entities rather than raw config in the patch attached.
Comment #90
alexpottA test has got in using
\Drupal::config()
.Comment #91
alexpottCreated the change record https://www.drupal.org/node/2407153 - please review / improve :)
Comment #92
Gábor HojtsyAdded a bunch of text on the dangers averted as well as the reasons this is being done and how the form code helps with doing the right thing. Also added a whole new section on form altering and what happens there. See https://www.drupal.org/node/2407153/revisions/view/8013861/8014005. Also mentioned the exception in that last section, but we can mention it sooner as well. I think this way the text explains the problem in more detail. It may repeat itself a bit, but I don't think that is a problem per say. Its a difficult problem and worth explaining from multiple angles.
Comment #93
effulgentsia CreditAttribution: effulgentsia commentedIssue summary, change record, and patch all look great. If a committer would prefer to commit #2407093: Refactor ConfigurableLanguageManager::updateLockedLanguageWeights() to save configuration entities rather than raw config separately, then this will need a reroll after that lands. Otherwise, since the patch there is small, non-disruptive, RTBC, and included in the latest patch here, I'd be fine with this being committed as-is, and then that issue closed. Therefore, RTBC.
Comment #94
alexpottThis critical has been traiged by @xjm, @webchick, @catch, @effulgentsia and me.
See:
[Edit: to show where in the issue some of this discussion is represented]
Comment #96
effulgentsia CreditAttribution: effulgentsia commentedClarified title and summary to cover config entities.
Comment #97
catchHave been reviewing this as it goes, and while this doesn't feel completely comfortable I think it's definitely the best we can do at the moment.
Extremely minor nitpick fixed on commit:
This is going to require contrib modules to update so better to get it in sooner than later.
Committed/pushed to 8.0.x, thanks!
Comment #99
larowlanPublished change record
Comment #100
XanoShiny! Now we have ConfigFactoryInterface::getEditable(), do we still need the config helper methods on form base classes/traits? I'm asking because it looks like if you get a config object in a form with the intention to update with saved form values, using ConfigFactoryInterface() won't work and ImmutableConfig will throw useful exceptions anyway.
Comment #101
Gábor Hojtsy@Xano: the config() method on the config form base class is exactly there to help you load the right version with little effort. See the form example in the change record: https://www.drupal.org/node/2407153
Comment #102
XanoI am familiar with that method. However, ConfigFormBaseTrait feels like we have painted ourselves in a corner a bit, especially because getEditableConfigNames() means one cannot load a mutable *and* an immutable version of the same config object. I admit I have not yet run into problems, but I can imagine situations where one would like to get both variants of a config object for things like inline previews.
TLDR; Because these helpers are not all optional, it doesn't just feel like we're giving developers advice, but also that we're forcefully making them do things our way.
Comment #103
Gábor Hojtsy@Xano: nothing makes it impossible to use the injected config factory if you need the same object for other reasons. I don't think we force people into a corner, they still have the injected config factory handy to do whatever they want. The resulting code will make it evident if they explicitly used it yes.
Comment #104
XanoAh, I see. The injected config factory used to be private, but that's no longer the case. That solves most of my concerns. I don't like the fact that getEditableConfigNames() is abstract, though. Is there something we can do about that?
Comment #105
Gábor Hojtsy@Xano: ConfigFormBase is for editing configuration. If you want to implement a form that does not edit configuration, you can use FormBase. If your form edits configuration, it will have at least one key that it would take as editable. The trait has no way to know what that key will be so it can only define it as abstract.
Comment #106
XanoI know what ConfigFormBase is for and I loved it until it introduced an abstract method for something I do not need. That's why I asked if we can undo this DX regression in such a way that we keep the trait functionality that people apparently like.
Comment #107
Gábor Hojtsy@Xano: You have a ConfigFormBase form and you do not need to edit configuration in that form? What do you use it for?
Comment #108
XanoConfigFormBaseTrait does not add any value to ConfigFormBase in my opinion. I do not like it and prefer not to use it, but instead inject the config factory and call it myself. However, that is not what I am trying to discuss here.
I like that ConfigFormBase lets met create a config form that looks like any other config form, but if I want to continue using ConfigFormBase for this, I will now also have to implement a method that I don't need, simply because others feel it makes their life easier. While making lives easier is an admirable thing, I don't like that this particular situation makes my life harder and I would like to find a solution for that. For instance, can we provide a default implementation of that method?
Comment #109
Gábor Hojtsy@Xano: that its necessary to implement getEditableConfigNames() is exactly to make you not go into that trap where you get non-editable config that you cannot save, which you may band-aid by using getEditableConfig() in your submit handler (because that is where you get the exception), which will "work" but will spill overrides into config possibly leading to security issues and go exactly against the goals of this solution. If we un-abstract it then that is what we get. Feel free to use FormBase and inject a config factory for your use.
Comment #110
effulgentsia CreditAttribution: effulgentsia commented@Xano: perhaps open a followup for making ConfigFormBaseTrait::getEditableConfigNames() not abstract and default to returning an empty array? My current thinking is that I agree with Gábor that it's not a good idea, because extending ConfigFormBase without providing at least one form element that edits some configuration isn't good, and having a form element that edits configuration without using $this->config() in both the #default_value assignment and the submit handler to ensure consistency across the two also isn't good. But current thinking can always change. Another possibility to consider in that follow-up is maybe making the default return value some kind of marker that says "all"?
Comment #111
alexpottKnowing what configuration if being used in which forms is also useful for translation see #2095289: Make getEditableConfigNames() public and use it in config_translation
Comment #112
YesCT CreditAttribution: YesCT commentedComment #114
Gábor HojtsyThis issue missed a form alter in locale: #2414279: Interface translations directory: Can not set values on immutable configuration.
Comment #115
vijaycs85Looks like this one in comment missed as well
Comment #116
vijaycs85Here is a patch for #115
Comment #117
catchPlease open a new issue for the follow-up to preserve issue metadata and commit history.
Comment #118
vijaycs85Sure, Here is a followup #2423555: Fix a leftover comment to reflect "immutable" config objects change
Comment #119
XanoComment #120
xjm