Problem/motivation
There are common APIs to access configuration across different parts of Drupal:
1. FormBase::config() proxies your call to the configuration factory.
2. ConfigFormBase::config() does the same but ...
The behaviour is very different. ConfigFormBase::config() will disable all overrides with the assumption that you deal with a configuration form to display and save non-overriden configuration. The original behavior of FormBase::config() is overriden, so there is no direct way to access configuration with all overrides applied.
Let's say you have an admin form that sends an email when a setting is updated. It sends to the site email address. You override that of course in $global['conf']['system.site']['mail'] = 'localdev@example.com'; so your customer will not get the emails from your developer site. You don't want this local override to get into the active configuration so that it is not applied when you edit admin settings is good. However in the form that sends email, it will also not be applied for sending the email. So pooof, you sent emails to your customer even though there is nothing evident in your code, that the config() method that in the other class applied overrides did not apply overrides in this class.
This is an example that some people mentioned, but I suspect there are probably other examples that I did not think of yet.
Proposed resolution
A. Make global overrides apply all the time. This would make local dev overrides end up in active configuration so not sure developers want to need to review those changes all the time?
B. Make the API explicit about which config methods return possibly overriden values and which return raw original values all the time. (Note that this may have side effects for how we solve entity listings, see #2136559: Config entity admin lists show configuration with overrides (eg. localized)).
C. Decide this is not a big problem. We are not concerned with dev environment emails from admin forms and other possible side effects that may come.
Remaining tasks
Discuss.
User interface changes
None.
API changes
TBD.
Related Issues
#1929136: Fix override-free context, move global config overrides back to an event listener
#1763640: Introduce config context to make original config and different overrides accessible
#1646580: Implement Config Events and Listeners, and storage realms for localized configuration
#1932336: Use override free configuration context for SystemConfigFormBase
#2136559: Config entity admin lists show configuration with overrides (eg. localized)
Comment | File | Size | Author |
---|---|---|---|
#84 | 1934152-live-vs-raw-config-84.patch | 29.64 KB | Gábor Hojtsy |
#84 | interdiff.txt | 657 bytes | Gábor Hojtsy |
#83 | 1934152-live-vs-raw-config-83.patch | 29.64 KB | Gábor Hojtsy |
#82 | 1934152-live-vs-raw-config-82.patch | 53.48 KB | Gábor Hojtsy |
#79 | 1934152_79.patch | 53.64 KB | martin107 |
Comments
Comment #1
Gábor HojtsyPosted this to the security team:
And tweeted this to get more people from the larger community: https://twitter.com/gaborhojtsy/status/308971595563679744 (earlier in https://twitter.com/gaborhojtsy/status/308969857951608832 which was removed due to a spelling error).
Hope this gets us some insight into what people want :)
Comment #2
Gábor HojtsyComment #3
DamienMcKennaBeing able to override settings per-hostname is extremely important for many developers and many sites. The settings.php $conf array is how I ensure that production sites have the correct production-worthy settings enabled while my local install has all caching disabled, uses alternative API keys, etc. This must be possible. How can I help? :-)
Comment #4
DamienMcKennasorry.
Comment #5
Gábor Hojtsy@DamienMcKenna: yeah, so overriding for different host names is an interesting use case. Do you mean overriding conditionally in settings.php or just using different settings.php files in different environments? global $conf is still used and applied EXCEPT if the code is in an "override free" environment which is only applied for admin forms. So if you go edit your site setup, your $conf overriden values will not be present there and not saved. Which is logical if you use logic in settings.php to apply to different domains, but probably irrelevant if you just use static settings.php overrides on different setups.
Comment #6
DamienMcKenna@Gábor: I've used multiple combinations of this:
I've had a blog post kicking around in my head for 2+ years on how I handle variables, I really need to write it down..
Comment #7
Gábor HojtsySo the question is if its a bigger problem if those overrides creep into settings forms and get saved as original config on sites, or if its a bigger problem if they don't creep into forms but then they are not applied when editing those config (so eg. if the global override changes the theme that would not be possible on the admin editing form). Which is the bigger problem? :)
Comment #8
Owen Barton CreditAttribution: Owen Barton commentedI have done many of the same things as @DamienMcKenna - I think the API key use case is one to consider for sure - how to we ensure that "production only" config doesn't leak to staging and vice versa (you don't want to get your real and testing Paypal API keys mixed up, trust me!). If there is not already a good way to ensure this, $conf might be a reasonable solution.
I have also used the case of having multiple hostnames all using sites/default (and the same database/files etc) and using $conf to switch out some key variables (frontpage, language, site name etc) based on the hostname. This is all stuff I could have done (less efficiently) with using sites/hostname or other means however.
Another use case to consider is for automated hosting setups - allowing the host to set the database and reverse proxy configuration dynamically (as Acquia does, for instance), and if necessary change it - it seems preferable if this can be done without needing to trigger a config reload or other operation that may have side effects.
Comment #9
catchAnything that can't actually be stored in the database/CMI is in $settings now - this distinguishes between things that are exclusively configured in settings.php vs. overrides. There's likely a grey area where something is used after CMI becomes available but you don't ever want a UI for it, I'd lean towards $settings for those too.
I think it's OK that conf overrides don't show up in forms and aren't saved back, that's arguably a bug fix compared to Drupal 7, there's two things that could use clarifying though:
1. Should we warn people on the form somewhere that values are overridden in $conf and their changes won't take effect?
2. We should put a huge warning on the 'no context' context to make it clear that $conf overrides are disabled for that context. I can't really think of a reason to use it except for the forms and maybe the new drush vget.
Comment #10
DamienMcKenna@Gábor: With my dev workflow, nothing is changed via the admin UI on production, so these overridden settings being exported to the config files elsewhere doesn't matter, the changes won't be committed unless I've reviewed every single line and given it my blessing.
Comment #11
attiks CreditAttribution: attiks commentedmy 2 cents: I use it a lot to differentiate between environments (all running on top of aegir)
Some things that can be in there besides API keys:
Comment #12
agentrickardI do address a similar issue over in Domain Access, where I have a small hook that can be used to declare which admin forms contain domain-sensitive values. In an ideal world, the config system might detect these hard overrides and flag them for the user via a message.
That would, however, involve some trickery to match $conf values with form config values. Perhaps if we disallowed putting "loose" $conf variables in settings.php and instead allowed for a standard function call for overrides:
e.g. Drupal 7:
Drupal 8:
Then the config form process could invoke settings_override() and look for matches.
/me stops thinking out loud.
[edit] And notes that catch said something similar in #9 while I was writing this.
Comment #13
Gábor HojtsyI don't think there is a common element in config forms, etc. views, node types and site settings don't have much in common, also form structure is not related to config structure (as in views, or even the 403/404/front path config is entirely differently presented on the form vs. the config file). It was a point of the CMI initiative *not* to deal with automated form generation. D8MI needs to generate such forms, so we have http://drupal.org/project/config_translation which was so far deemed not a fit for Drupal 8 core. There it is possible to mark elements, because all forms are autogenerated from the schema/data combined. I don't see a reliable way to mark elements in Drupal forms overall where original config is edited.
Comment #14
moshe weitzman CreditAttribution: moshe weitzman commentedI think it is a big win to disable form elements where the admin's changes would not make any difference. It is not appropriate IMO to offer a non-overidden value for editing without telling the user whats going on. His edit is likely not to make any difference. We have always done it that way and it has always sucked :(
If config_translation project gets us there, perhaps we should consider it for core.
Comment #15
Gábor Hojtsy@moshe: I'm not sure such programatic backreferences are possible. In Drupal 7, system_settings_form() pages were known to store their values in variables and their form elements were named equal to the variables. So you could look at the form element name and put on warnings if it was overridden. Eg. variable module used this to put on notes if the variable was multilingual.
In Drupal 8, there is no central store like variables, and it is not really known where in config would the value be stored. So while you can get the form element name, you don't really know where in config it would end up. For example the user account settings page uses two config files to store settings, one for "base" settings like anonymous user name or whether registration is allowed and one for all the user email subjects and bodies. All are edited on one form altogether. Menus are config entities and the menu editing form merges editing the menu itself with menu items, so it is quicker for the user. So to be able to tell for specific form fields whether they are overridden (globally or in some language), we'd need to have this information on each form element. That is lots of added info to form structures, that is not there now. And then finally, the question of how we generally disable an element comes up. There are potentially complex form elements. Eg. if the site logo upload field would have an inline preview of the logo, how would you make that form element disabled? If you enforce date formats to be in a specific setup, then multiple admin pages would be entirely disabled / pointless. You could try to add new formats, but that would not work.
The config initiative specifically opted to not deal with automated form generation and wanted to leave forms as one-off implementations on top of config. Later config entities somewhat structured this for whatever is implemented through config entities, so you kind of have some structure (and config vs. form relation somewhat programatically discoverable), but still then the form item vs. config key relation does not necessarily work. See menus.
What config translation (contrib) module does it takes the config schema, that describes the structure of the config and generates translation forms based on that. It totally ignores the original forms for config, because it just cannot pull out fields from the various views forms or from the middle of a config entity or cherry-pick elements from different levels from the structure of the user account settings form. So that does not even attempt to do this relation, because it looks pointless. Dries spelled out earlier he sees this module as a parallel form API and not a fit for Drupal 8. Also, this module needs to build up its own relation system between admin paths and config keys, so it can put translate tabs on the right places for example (and go and alter random forms of various structures to put in translate operations to summary tables).
Comment #16
moshe weitzman CreditAttribution: moshe weitzman commentedOK, you've convinced me that showing disabled form elements is desireable, but impossible. Sigh.
I guess we need an "override free" state so that we don't save translated values of site slogan and config like that. Can't we just neuter the LocaleConfigOverride in this case? In my world, all conf overrides would *always* be in effect, even during admin forms. This preserves D7 behavior and satisfies the security concern described by @pounard.
Comment #17
Gábor HojtsyYes, it is totally our decision, whether we want to make global overrides always apply for example, if those showing up in forms is a smaller issue vs. those not being applied on some admin forms. If we add a feature so overrides can optionally apply in override free contexts too, then one can do logic based overrides (such as domain based overrides in contrib) with a system separate from global $conf and it is their choice if it applies in this context or not. We will need a better name for this context then too :)
Comment #18
David_Rothstein CreditAttribution: David_Rothstein commentedI believe I just ran into a version of this, and the current behavior is bizarro broken.
You can look at #1926228-5: Performance page provides incorrect/incomplete information about CSS and JS compression for a really simple patch I tried to write which changes the value of some text on an admin form based on a config setting (one which we encourage people to set in settings.php). The patch didn't work and I assume this "don't apply overrides on admin forms" behavior is the reason?
Or here's a simpler example you can try:
$conf['system.site']['name'] = 'My Drupal site';
line.Yeah...
Comment #19
Gábor Hojtsy@David: sounds like the reason indeed. Looks like one more +1 from you to make global overrides stick. (Considering them creeping into forms not a bug like many people noted but a feature like many other people noted). Here is a possible solution. Will probably not pass tests yet.
1. I kept the override-free context, since it is used in internal config operations to access the original values for import & staging (see code in core/config.inc exclusively, which is not even touched in the patch). With this patch that is the only place where that is used.
3. Added the capability to force overrides. The default override handler will take any override either forced or not. The free context will not take any override, even if forced (may be a possibility for misunderstanding :) and there is a forced only context, which only takes forced values.
2. Added a config.context.only.forced on the DIC to expose this and made the admin forms use it. Made global overrides forced.
Comment #21
Gábor Hojtsy@Moshe:
Domain module will also use this override system. The overrides of domain module are conditional on which domain you are on, so saving them back to the original config might be an issue or at least confusing. There are unconditional overrides like sometimes global $conf (although I've seen conditions in settings.php files when setting config values in $conf), and there are dynamic overrides like translations or domain overrides. Looks like we want some of them apply to forms but not others. So the question is where is that decision made. The patch I proposed puts that decision with the override event listener and only the global overrides decide to "apply globally" in the patch. The domain overrides could then decide in contrib to apply globally (except when truly no overrides apply in internal config operations) or not.
Comment #22
Gábor Hojtsy#19: global-overrides-forced-19.patch queued for re-testing.
Comment #24
Gábor HojtsyIt helps if I follow the new interface definition in implementations :)
Also as for why the overrides creeping in to forms and therefore current config is a problem, if you have eg. domain module, depending on which domain you save your admin form, the config saved will be different, if the domain conditional config override gets into the form. That will be confusing when you stage / update site config from development, since your live site will show changes while it just flip-flops between different domain based values. The config diff / staging system as well as overrides were built with the idea that the saved configuration is the canonical value and overrides are not used as "even more canonical".
Comment #25
Gábor HojtsyAnybody still interested in resolving this? It has been raised to critical, a solution has been proposed and then no feedback for over two weeks. If that is as much as we care, then I don't see how this would be critical?
Comment #26
tstoecklerI'm not sure how entirely, but I think #1932336: Use override free configuration context for SystemConfigFormBase is related to this.
Comment #27
Gábor HojtsyYeah, that would also need to be modified to the new context introduced here. I don't think it makes sense to keep updating the patch if nobody cares though.
Comment #28
Gábor HojtsySounds to me like if people care so much as to not review the proposed patch for a month, then this should not be anywhere near critical.
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedadding tag.
Comment #30
Gábor HojtsyHah, heh. Any opinions? :D
Comment #31
Gábor HojtsyTalking to people in Portland, they thought this is still very important, they just did not have time to think about this. So this can just as well be back to critical.
Comment #32
Anonymous (not verified) CreditAttribution: Anonymous commentedi've tried to think of something useful to say about this issue and patch a couple of times, but never quite managed it.
implementation details aside, i'm ok with values set in settings.php being special, so i think i'm ok with the patch in #24. the context system is already insanely hard to reason about, so i don't think another layer of special casing makes much difference, and seems to solve a real use case or ten.
honestly, i'd still rather we rip out the context system, and bake in special handling for settings.php and locale. that would get us more features than D7 (first class locale support), without making it insane.
Comment #33
mtiftI can no longer duplicate the behavior outlined in #18. However, the idea of making global overrides stick makes sense to me.
It looks like this patch needs a re-roll now that CoreBundle.php has been replaced by CoreServiceProvider.php in #1988508: Stop pretending we support Symfony bundles when we really just have service providers.
Comment #33.0
mtiftUpdated issue summary.
Comment #33.1
mtiftUpdated issue summary.
Comment #34
Ec1ipsis CreditAttribution: Ec1ipsis commentedComment #35
Ec1ipsis CreditAttribution: Ec1ipsis commentedPer my review with @YesCT, this patch is likely in need of a complete rewrite. Maybe @Cottser can help?
There are three files with issues: core/modules/system/system.admin.inc, core/modules/user/user.admin.inc, and core/lib/Drupal/Core/CoreBundle.php. User.admin.inc in particular had ~1,700 lines deleted from it, and as was mentioned above CoreBundle.php was completely removed.
We attempted to find the issue that deleted CoreBundle.php with the command: git log -S'class CoreBundle extends Bundle'. The intention was to find the commit where that particular line was removed, which would tell us when the file was deleted. The result was:
Unfortunately, this didn't provide the information we were after.
The attached patch is the result of applying #comment-7189198 and removing all offending code. File size of new patch has been reduced roughly 60% due to changes being removed during the process.
Comment #36
moshe weitzman CreditAttribution: moshe weitzman commentedAnyone able and willing to take this on?
Comment #37
moshe weitzman CreditAttribution: moshe weitzman commentedAnyone able and willing to take this on?
Comment #38
Gábor HojtsyMoshe: I think we should discuss first and foremost what we want. Not many people cared to comment on my patch :/ Also #2098119: Replace config context system with baked-in locale support and single-event based overrides is about to change the config context/override system drastically so not sure there is a point in working on this one *now* until that is committed.
Comment #39
DamienMcKennaA minor reroll.
Comment #40
DamienMcKennaLetmetrythatagain.
Comment #41
DamienMcKennaPutting it back on the backlog per #38.
Comment #42
Anonymous (not verified) CreditAttribution: Anonymous commentedbump.
Comment #42.0
Anonymous (not verified) CreditAttribution: Anonymous commentedUpdated issue summary.
Comment #43
Gábor Hojtsy#2098119: Replace config context system with baked-in locale support and single-event based overrides landed. Still need to figure out if we want to do this.
Comment #44
mgiffordThis no longer applies....
$ git apply drupal-n1934152-39-conf_ftw.patch
error: patch failed: core/lib/Drupal/Core/Config/Config.php:275
error: core/lib/Drupal/Core/Config/Config.php: patch does not apply
error: core/lib/Drupal/Core/Config/Context/ConfigContext.php: No such file or directory
error: core/lib/Drupal/Core/Config/Context/ContextInterface.php: No such file or directory
error: core/lib/Drupal/Core/Config/Context/FreeConfigContext.php: No such file or directory
error: core/lib/Drupal/Core/EventSubscriber/ConfigGlobalOverrideSubscriber.php: No such file or directory
Comment #45
BerdirIs this still an issue given that the context stuff is gone and this is always applied last in ConfigFactory?
Comment #46
alexpott@Berdir The question that remains is: should ConfigFactory::setOverrideState(FALSE) stop overrides in settings.php from applying? People have raised this as a security issue.
Comment #47
Gábor HojtsyRight. I have not managed to get the security team to weigh on this though :/
Comment #48
webchickComment #49
chx CreditAttribution: chx commentedNow that context is gone the IS definitely needs upgrade. As there doesn't seem to have anyone else, I will try to follow up with security considerations (someone must, after all) once that's done.
Comment #50
Gábor HojtsyComment #51
Gábor HojtsyUpdated the issue summary! The situation now in short is that global overrides always trump every other override, see ConfigFactory::get(), but no override is applied if in override-free mode, such as in admin forms, see ConfigFactory::setOverrideState(). So this later situation is in question in this issue.
Comment #52
Gábor HojtsyComment #53
chx CreditAttribution: chx commentedYes, settings.php overrides must win. I am not sure how is this even a question but having access to settings.php is usually rarer than having access to the UI so it should win.
We discussed the UI half with timplunkett and the conclusion I arrived to is as follows:
Comment #54
Gábor HojtsyYeah that as an idea is great (to show if a value is overridden) *however* configuration does not have a consistent formbuilder API at all for anything outside the form class itself to inject generic messages and/or disable elements. Configuration does not even use standardized widgets so we can tell a widget to disable itself (eg. parts of the views UI or a block configuration or whatnot). So long as each form needs to deal with any part of it needing to be disabled and print that message, it becomes a huge pain.
Finally, even if all forms would deal with that and display such message, it may or may not be true. There is no indication as to whether the global override is static or its dynamic, ie. it may or may not apply at certain cases and the db stored value may or may not apply then with appropriate conditions. Eg. if config is overridden for anonymous users, the admin will never see the override on the UI and the changes will be possible to be saved, but not taken for anonymous users. Same if the config is overriden for authenticated only, the config cannot be changed in your scheme even if it would change how anonymous behaves.
One of the things pointed out in the issue summary is that global settings overrides are used for all kinds of fun things, eg. site sections / context dependent settings which comes with various conditions.
In short I don't think we can do anything about this on the UI since there is no control over how the UIs are created in the first place. AFAIS the only question is whether we apply those overrides for the UIs too, which seems to be people saying NO, since it would save back to actual config and with the assumption that the global settings overrides are environment specific (another interesting assumption about what people use their global overrides for :D), we don't want them to end up in that config.
Comment #55
chx CreditAttribution: chx commentedSure, skip the UI parts, I just didn't want to confuse people by showing one value in the UI and then editing it not taking but then again I presume settings overrides will be rare and understood.
Comment #56
Gábor Hojtsy@chx: well, the state of play now is that overrides are not applied to config when taken for forms following #2251915: Overridden config entity bleeds through to admin forms and #2239065: Overridden config bleeding through to configuration forms. From one point of view that is good because the overrides will not appear in the forms and therefore not saved back to config storage. That is what @webchick opened #2239065: Overridden config bleeding through to configuration forms as a critical for.
OTOH now the entity loaded in upcasting in admin contexts and all config loaded on forms is override-free and therefore if you wanted to apply overrides to eg. configuration of how the form is built or how the form acts when submitted that is impossible to do now because the overrides don't apply. Eg. if the admin form sends an email when changed and the email address is overridden in the local context so the live monitoring is not spammed, this will not work because admin forms don't apply overrides you know.
This issue was opened for security considerations. I don't have a good example, but surely there are people who are more creative than I am :D
Comment #57
alexpottHmmm... we should only be using override-free configuration for what is displayed & configured using the form - not for how form acts.
Comment #58
Gábor HojtsyYeah that sounds good as a principle. However:
1. In practice since we have all that code doing $this->config(), it is very easy to use $this->config() for whatever builds the form or reacts to the submission as well.
2. OTOH any API calls to get data for forms (ie. not using $this->config() to get data but some API) may get us overridden config, since our override state altering is entirely encapsulated in ConfigFormBase::config().
So forms may still get overridden config where they did not want (2) and they would naturally use $this->config() for auxiliary config access even for things where overrides should be applied (1). I think the distinction is where overrides should be applied is a big enough can of worms that the multiple use cases of global overrides just complicates more.
So looks like it would be down to developers understanding the whole override mess for them to make informed choices, no? We can make parts of the API easier to identify where they belong. Eg. we can rename ConfigFormBase::config() to rawConfig() or something along those lines? Not sure how to make it apparent that admin upcast entities are also raw but all other API access will not yield raw config and that for behavioural code in the form one should not use raw config.
Comment #59
alexpott+1 to making it more explicit through renaming the method - great idea.
FormBase implements config() too - so I think the ConfigFormBase should actually throw an exception if that is called. And then we can have two methods, not sure of their names... ConfigForEditing() and ConfigForUsing() so that the caller knows the explicit choices they are making.
Comment #60
Gábor HojtsyHere is a draft for what that may look like with rawConfig() and liveConfig() as method names. I just did a quick phpstorm refactor so there may be false positives or missing pieces. Just posting to keep the discussion going. The substantial change is in ConfigFormBase.php
Comment #61
Gábor HojtsyAny feedback on this approach to make the override/no override situation clearer in config admin forms?
Comment #62
catchNot 100% on the method names, but having two explicitly named methods seems good.
Should 'raw' be 'stored' maybe?
Comment #63
Gábor HojtsyConfig also has
it also has getOriginal() which is the (not yet modified from) stored version of the config with or without overrides :) So we should somehow look for terminology harmony here and if we want to introduce something else here, then apply it to elsewhere where it makes sense.
Comment #64
YesCT CreditAttribution: YesCT commentedwhen will we use liveConfig()?
it is not used in the patch.
and I'm not clear from the docs.
yeah, consistant names through Config StorableConfigBase and ConfigBase would be helpful. If we can agree on an approach here, we could do renaming across the classes in a separate issue.
Comment #65
Gábor HojtsyliveConfig() would be used on any config that is read to construct the form and/or to react to the form being submitted outside of saving the config itself. That is if you are editing config A, you may use config B to construct the form or react to saving it. I'm not sure replacing config() methods on base classes with liveConfig() for consistency is a good idea in and of itself though.
Comment #66
YesCT CreditAttribution: YesCT commentedI was thinking more along the lines of consistent use use of: raw (stored) original live data
I suppose I could clarify by actually making such an issue, and if we decide not to, wont fix it.
Comment #67
Gábor Hojtsy@catch: what do you think in light of the mentioned other existing methods?
Comment #68
star-szrRerolled for PSR-4. One small context conflict in \Drupal\Core\Installer\Form\SiteConfigureForm.
Comment #71
mgiffordsadly...
Comment #72
chx CreditAttribution: chx commentedOne of those patches where using git to reroll is significantly easier: the patch applied on top of d0d3e533615df7539a54cac995b50e75f18d1597 then I merged on top of HEAD and there was a single, easy to fix conflict.
Comment #75
Gábor HojtsyThanks @chx for rerolling. This is still down to needing feedback.
Comment #76
Gábor HojtsyComment #77
YesCT CreditAttribution: YesCT commentedmaybe formstate interface conflicts... july was a long time ago.
Comment #78
catchTagging with D8 upgrade path since we're aiming to fix all known security issues before supporting that.
Comment #79
martin107 CreditAttribution: martin107 commentedThe age of the patch, plus the number of changes I made, make me think I will not get it all completely correct first go
#77 is mostly right ... lots of form state changes....
I mades note .... MethodForm.php and NegotiationConfigureForm.php are files where the code had changed so much that I could just go with the changes from HEAD.
Lets see what testbot says.
I will lurk on this issue until its green:)
Comment #81
martin107 CreditAttribution: martin107 commentedIt looks like duplicates of what was defined in FormBase
public function config()
public function rawConfig()
public function liveConfig()
will now need to be defined as trait, which can also be used in \Drupal\Core\Entity\EntityForm.
that way lines that involve $this->rawConfig() will work in things that extend EntityForm and things that extend ContentEntityForm.
For example look at the test failure for
\Drupal\contact\ContactFormEditForm. ( which extends EntityForm )
\Drupal\comment\CommentForm ( which extends ContentEntityForm )
This is a critical issue and needs input from system architects before I proceed :(
So after putting on my flame resistant suit t can I start the naming debate by suggesting
ConfigHelperTrait
Comment #82
Gábor HojtsyYour patch contained unrelated stuff, like that MenuSettingsForm. I redid the patch from scratch with the following:
- same changes in ConfigFormBase and FormBase
- phpstorm refactor to rename config() to rawConfig()
- I removed the config() from ConfigFormBase since it was not in use anymore
The goal of this issue to let config edit forms access raw config by default. So long as you are not **editing** your config, the primary use case is you want to access it with all overrides applied, in fact that distinction was the goal of this issue. So I am not sure why would content entity forms need this at all? They don't currently have a config() method and if they would have one, they would expect to get values with overrides (they are not editing that config).
Comment #83
Gábor HojtsyNote that 82 also renames the config() method to rawConfig() in FormBase, which is silly, that is a side effect of the overzealous phpstorm refactor tool :) We should not do that of course. Here is another more intelligently ran refactor that keeps config() on FormBase intact. No interdiff because I recreated the refactor with phpstorm again. The actual changes are only in ConfigFormBase the others are refactor (search and replace).
Comment #84
Gábor HojtsyDuh, given the parent is not rawConfig() anymore, we should call the right method. Also minor docs fix.
Comment #85
chx CreditAttribution: chx commentedComment #86
Gábor HojtsyI don't think its a smart idea to work on more code for this unless we even agree we want something like this. Sounds like an easy waste of time for someone. The tests will not make the approach any easier to review or provide feedback on. We still need feedback on this. I don't contest that it needs tests once we agree on even doing this and doing it this way. Therefore moving back to needs review for feedback.
Comment #87
dawehnerI kinda like semantic exceptions ... have you thought about using
BadMethodCallException
instead?The only question is why we not just have liveConfig() and keep config() as it is. I would assume that the common usecase for settings forms is exactly the functionality of rawConfig?
Comment #1934152-58: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects and #1934152-59: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects seem to be the key points here. We want people to actively make the decision. Did we considered to have
rawConfig()
aswell as
overriddenConfig()
(maybe this tells a bit more about what is going on), but just an idea ...Comment #88
Gábor Hojtsy@dawehner: Thanks for the feedback. I definitely agree a more semantic exception is in order. I will work on that as soon as there is some agreement we want this kind of change to happen :) You also identified the reasons for the two new methods very well. I think overriddenConfig() may be a bit confusing, as we don't really know if there are any overrides, there may be... on an English only site that does not use domain access, og, etc. there will not be anything that may even override config. So that was the reason for liveConfig(). But I don't have very strong feelings on that, overriddenConfig() is fine if others believe that is fine too :)
Comment #89
benjy CreditAttribution: benjy commentedI really think having global overrides from settings.php is important. We use it for all kinds of things such as disabling cache, css/js aggregation locally, API keys on live etc.
Didn't Acquia just use this global override approach to switch out the DB abstraction for the recent security issue? Yes you might do that differently in D8 but I think that shows that a way to trump all other configuration from the settings file is very handy in many cases.
Comment #90
Gábor Hojtsy@benjy: right, this patch does not make them applied less, it just makes it obvious when overrides are used vs. when they are not, so people can consciously use what they need to. It is important that when you edit config, you can edit the original values and that experience is not obscured by overrides (for domain, language, etc). However for anything else on the page, such as eg. labels of the forms for editing the config should apply overrides. Eg. if you are editing a view in French, the UI should be French (eg. labels of fields) but if the view is originally English, you should edit the English view not a (partial) translation.
Prior to this patch it is easy to just use config() and depending on whether your class was extending ConfigFormBase or not, it would behave VERY differently. The goal of this patch is to make that behaviour difference apparent.
Comment #91
tim.plunkettIs this still in the "figure out if" stage? Perhaps this could be discussed and decided in Ghent?
Comment #92
xjm@tim.plunkett, yep, it's going to be discussed today. See: https://groups.drupal.org/node/451513
Comment #93
Gábor HojtsyComment #94
Gábor HojtsyComment #95
Gábor HojtsyComment #96
Gábor HojtsyPostponing on #2392319: Config objects (but not config entities) should by default be immutable based on discussion in Ghent. That would provide a systemic solution that also applies if you use FormBase to save back config (eg. you don't know about ConfigFormBase) or for things like config entity list controllers that set weights which have the same problem
Comment #97
xjm(For the issue summary update and hard problems discussion.)
Comment #98
Gábor HojtsyComment #99
Gábor HojtsyComment #100
alexpott#2392319: Config objects (but not config entities) should by default be immutable will probably solve this issue - if it does I will mark this issue as a dupe. OTOH, if this does not play out that way then we will need to triage this issue.
Comment #101
Wim LeersComment #102
Gábor Hojtsy#2392319: Config objects (but not config entities) should by default be immutable resolved this in a different way. Config forms config() will work appropriately for config names if they are listed in the editable config method or not.
Comment #103
Gábor HojtsyComment #104
xjmComment #105
sime