8.x 1.x config ui domain switch #294

davidgrayston · 2016-11-29T14:03:18Z

#15 proof-of-concept

Allows switching the active domain from a "sticky" form
Saves configuration for the selected domain

…immutable config

zerolab · 2016-11-29T22:51:02Z

Looks promising. A few general comments:

DI for the domain.negotiator service
Does this work with settings overrides?
Needs tests.

edit: removed line about moving to modules. PEBCAK

davidgrayston · 2016-12-03T22:52:36Z

@zerolab regarding your comments:

DI for the domain.negotiator service

I've now done this, I hope in the correct way

Does this work with settings overrides?

I intentionally didn't load the settings overrides as it might be confusing when editing.
I'm now only re-applying the module overrides on non-immutable config so that the settings overrides will take effect when browsing the site, but config forms should still show module overrides.

Other issues:

I'm currently undecided if the window should reload when switching domain.
I think extra logic should be put in place to only apply domain module overrides on edit forms.

…o config-ui

agentrickard · 2016-12-06T21:25:14Z

I merged this to the config-ui branch for testing. First note, when enabled:

 ken ] : drush en domain_config_ui
The following extensions will be enabled: domain_config_ui, domain_config
Do you really want to continue? (y/n): y
Fatal error: Arrays are not allowed in class constants in /Applications/MAMP/htdocs/drupal-8/modules/domain/domain_config_ui/src/Config/Config.php on line 20
Drush command terminated abnormally due to an unrecoverable error.                                           [error]
Error: Arrays are not allowed in class constants in
/Applications/MAMP/htdocs/drupal-8/modules/domain/domain_config_ui/src/Config/Config.php, line 20
/Applications/MAMP/htdocs/drupal-8/modules/domain

agentrickard · 2016-12-06T21:38:50Z

I found (or, really couldn't find) the form selector.

The bottom right is not a natural place to find this element. That's the last place a native LTR reader would look for it.

Ideally, we'd be near the Save configuration button -- if not actively changing that button.

davidgrayston · 2016-12-07T09:14:10Z

Ah, array class constants are new in PHP 7, your fix looks better

davidgrayston · 2016-12-09T23:24:13Z

domain_config_ui/domain_config_ui.services.yml

+services:
+  domain_config_ui.manager:
+    class: Drupal\domain_config_ui\DomainConfigUIManager
+    arguments: ['@config.storage', '@domain.loader', '@language_manager']


I'm no longer extending the domain config overrider or negotiator. DomainConfigUIManager service will now manage the language/domain switching.

Was there a specific code reason that made doing so necessary?

Ideally, we'd handle all of the negotiation inside the DomainConfigOverrider, or, if needed, create a DomainConfigManager service for this.

My thought here is that the UI piece should not handle any of the negotiation logic, just the form handling, so that it could be swapped.

The reason was to keep the loading of config that's being edited as self-contained as possible.

Spoofing the requested domain for the negotiator meant that all the config on the page also got swapped. It didn't seem right to have an admin tool overriding the negotiator and change its behaviour.

I'm also not sure if we want setting overrides to be applied to editable values?

I can change this back if you think swapping all config on the page is better?

On second thoughts, I am swapping all allowed config with the manager so it might not be helping much.

I can change back to extending the negotiator as before and see what problems I encounter.

I've changed it back to use the negotiator on a new branch here https://github.com/davidgrayston/domain/tree/8.x-1.x-config_ui_domain_switch_negotiator

Diff with current pull request branch: davidgrayston/domain@8.x-1.x-config_ui_domain_switch...8.x-1.x-config_ui_domain_switch_negotiator

I had to make some changes to DomainConfigOverrider to make this work:

Setters to allow the domain and language to be set

Changed protected getDomainConfigName method to be public

This doesn't seem like the correct solution in its current state

I haven't had a chance to really dive into this code. The part that bothers me is the introduction of config handling to the DomainNegotiator. That class/service should not really be Config aware.

I have no problem making getDomainConfigName public, but I haven't worked through the pro-con.

In general:

The core Domain module and its classes should handle domain registration and negotiation. That's all.

Domain Config should handle storage and retrieval of config overrides.

Domain Config UI should put a form in from of the read/write aspects of Domain Config.

Put another way, we should be able to programatically write an override through Domain Config.

Due to the saving of configuration not being tied to forms in a standardised way, and there being no definitive way of knowing which forms/pages handle config, it's quite tricky to implement this.

I don't think there's a neat way of loading the config overrides only for forms values (and admin pages), so we have to swap the config on the entire page - it's essentially swapping the domain being viewed. This is why I initially made a global domain switcher (sticky form), to make it clear that you were changing the selected domain globally.

Now that I'm altering "known" config forms, the user will not know which domain is currently selected until they go to an admin form with the switcher field. Perhaps we should go back the the domain switch implementation but make the form much more prominent (e.g. a fixed strap at the top of the page)?

It might be quicker if you take these ideas as a proof of concept and implement as you have described above?

I am working on finishing other parts of the alpha/beta release, so speed isn't something I can provide.

I need to take a deeper look at what you've done and the problems you define and try to give some better direction.

Using a config_ui_manager to handle forms makes sense. I just don't want that to bleed over into handling save/load.

That's really tricky place, d.o core has a lot of "config override bleeding" issues fixed, also working on domain_lang module it became clear that priority of loading overrides highly coupled to language manager initialization.
Also some overrides can be done via settings.php and somehow that should affect in UI

davidgrayston · 2016-12-09T23:25:45Z

domain_config_ui/domain_config_ui.services.yml

+      - { name: service_collector, tag: 'config.factory.override', call: addOverride }
+    arguments: ['@config.storage', '@event_dispatcher', '@config.typed']
+    calls:
+      - [setDomainConfigUIManager, ['@domain_config_ui.manager']]


Config UI manager is being passed to the config factory so that it can be used to load up selected domain/language configuration.

davidgrayston · 2016-12-09T23:28:47Z

domain_config_ui/src/DomainConfigUIManager.php

+  /**
+   * Load only overrides for selected domain and language.
+   */
+  public function loadOverrides($names) {


Config UI manager loads overrides for the current selected domain/language

davidgrayston · 2016-12-09T23:35:11Z

domain_config_ui/domain_config_ui.module

+    ],
+  ];
+
+  // Add language select field.


Language can now be selected

…h form for unconventional admin forms e.g. theme

davidgrayston · 2016-12-10T02:48:53Z

domain_config_ui/src/Form/SwitchForm.php

@@ -0,0 +1,113 @@
+<?php


I've re-introduced the switch form for use on pages that aren't conventional config forms, such as the theme settings page. This form will reload the page when the domain is switched.

davidgrayston · 2016-12-10T03:00:39Z

domain_config_ui/src/Config/ConfigFactory.php

+        $cache_key = $this->getConfigCacheKey($name, $immutable);
+
+        if (isset($module_overrides[$name])) {
+          $this->cache[$cache_key]->setModuleOverride($module_overrides[$name]);


I've applied the domain overrides on immutable config so that unconventional admin pages (like theme settings) show the domain domain specific config

davidgrayston · 2016-12-10T03:05:53Z

I still need to go through the known paths/forms/config that should be added to the allowed list by default.

The /admin/config/system/site-information form seems to be working fine, but the theme pages are still a bit clunky to use at the moment.

andypost · 2017-02-13T21:13:33Z

Here's addition to show widget with permission only skilld-labs#1

dmiric · 2017-02-25T23:49:00Z

For the last few days before I saw this I was working on the solving this problem on my own. I'll try to describe my approach to this.

So what I did was:

Introduce a hook in config save (hook_config_alter) that accepts config name and data
Overridden config factory method getEditable to return overridden configuration values for the domain I'm currently on
And then In hook_config_alter I would simply change config that is being saved to have the name and data I wanted to save for that site

So far everything is working pretty well. I can edit all simple configuration blocks and views its really early to tell if I can achive everything I want this way but looks promising.

Of course I'm also not sure If Drupal community would accept to have that hook in config save.

What do you think about that approach?

If it was not clear enough. You basically visit the site from domain you want to change config and make changes in usual places as if you were building a normal Drupal site.

dmiric · 2017-02-25T23:51:44Z

I'm also thinking about making a domain template so you could inherit configuration from template and then introduce minor changes on domain it self so we don't have so many config files.

andypost · 2017-02-26T11:37:38Z

@dmiric Use events instead of hooks for config, see ConfigEvents core class
BTW we using this branch on a project and we are going to add language & number of overrides to the floating block, also looks toolbar is a proper place for that

dmiric · 2017-02-26T12:21:53Z

I tried to do it using config::save event, but I could not figure out how to avoid saving the base site configuration. Would you happen to know how I could do that?

…

On Sun, Feb 26, 2017, 12:37 Andy Postnikov ***@***.***> wrote: @dmiric <https://github.com/dmiric> Use events instead of hooks for config, see ConfigEvents core class BTW we using this branch on a project and we are going to add language & number of overrides to the floating block, also looks toolbar is a proper place for that — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#294 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABEu-PvU61T-zfXR2koX5CcYlD-GbeLWks5rgWQDgaJpZM4K_AYj> .

andypost · 2017-03-10T00:24:06Z

After trial on this UI following bugs found

domain override always picked from session (set by floating box), probably it needs to use query string argument to pass, so UI needs to redirect to the same page but appending URL with ?domain=ID
edit forms always using original config, overriden config factory probably should care to load override if exists
language in name of override makes a lot of overrides and not using config translation

andypost · 2018-01-28T06:50:21Z

One more core change in this area https://www.drupal.org/node/2930829

agentrickard · 2018-01-29T14:20:24Z

Hooray! That change was a long time coming.

rivimey · 2018-06-07T13:24:33Z

I created a new PR for this as #426 which (apart from unrelated 8.6 issues) merges and builds successfully.

andypost · 2018-06-07T15:20:02Z

Actually instead of 2 PRs better to create repo like we used to hack https://github.com/skilld-labs/domain_config_ui and re-do it

Core in progress of CMI 2.0 so that may affect domain_config itself (I still looking to move domain overrides to own collections instead of prefixing)

rivimey · 2018-06-11T09:23:03Z

@andypost Apologies, but I don't really understand "create repo like we used to hack...and re-do": I'm still finding my way around here.

[My intent here was only to use travis to check the patch again, given it has been a while since it was pushed. I intend to close the PR. ]

agentrickard · 2018-06-27T17:49:11Z

I'm going to pull https://github.com/skilld-labs/domain_config_ui into a new branch here for testing.

davidgrayston · 2018-06-27T18:27:59Z

Happy to apply the changes to the branch on this PR if that helps?

agentrickard · 2018-06-27T19:38:25Z

Well, I moved things to #434 so that it would be part of the main repo. So new changes would go there.

davidgrayston added 10 commits November 19, 2016 23:51

Config UI concept

e753e10

Starting domain switcher implementation

5ea6431

Showing domain specific config in admin forms

5ce0c3c

Always loading writeable config (for now)

8e97cf9

Fixing CSS

7979a74

Re-implementing doGet and doLoadMultiple methods to allow us to have …

4ee5266

…immutable config

Extending domain.negotiator rather than domain_config.overrider

f733a5a

Removing 'all' domain ID value

5b88098

Update README.md

704fd94

Merge branch '8.x-1.x' into 8.x-1.x-config_ui_domain_switch

8150320

davidgrayston added 9 commits December 2, 2016 21:26

Only allow domain administrators to switch domains

2fb2838

Only return selected domain ID on admin paths (for config forms)

9779330

Allowing global config to be altered

4ccd120

Merge branch '8.x-1.x' into 8.x-1.x-config_ui_domain_switch

f71ca8b

Injecting negotiator

dd5a870

Reverting restriction on admin routes

1fc15c9

Injecting the negotiator

c22d411

Removing unused use statement

963fa57

Only applying module (domain) overrides to editable config

f540d64

davidgrayston and others added 5 commits December 3, 2016 22:58

Removing override code that will never run

35da0ed

Only loading domain overrides for config forms

fb20fa2

Allowing wildcard config to be excluded

d0ee96b

Fixing typo in comment

41dbea4

Merge remote-tracking branch 'dg/8.x-1.x-config_ui_domain_switch' int…

59395ea

…o config-ui

Fixes constant handling issues; renamed $global_config to $disallowed.

a006b80

davidgrayston commented Dec 9, 2016

View reviewed changes

Removing unused method

564533b

davidgrayston commented Dec 9, 2016

View reviewed changes

domain_config_ui/domain_config_ui.module

],

];

// Add language select field.

Copy link

Contributor Author

davidgrayston Dec 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Language can now be selected

davidgrayston added 6 commits December 9, 2016 23:48

Displaying language in warning message

2fa1304

Removing unsused libraries config yml

71de60b

Allowing node.settings to be saved for a specific domain

d3b2534

Moved allowed config check to ConfigFactory + introduced domain switc…

a83d001

…h form for unconventional admin forms e.g. theme

Moved allowed config check to ConfigFactory + introduced domain switc…

597a02d

…h form for unconventional admin forms e.g. theme

Add warning message to the bottom of unconventional admin forms

6ef85ff

davidgrayston commented Dec 10, 2016

View reviewed changes

Added more allowed config and theme settings form

785b2c6

agentrickard closed this Jun 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8.x 1.x config ui domain switch #294

8.x 1.x config ui domain switch #294

davidgrayston commented Nov 29, 2016

zerolab commented Nov 29, 2016 •

edited

davidgrayston commented Dec 3, 2016

agentrickard commented Dec 6, 2016

agentrickard commented Dec 6, 2016

davidgrayston commented Dec 7, 2016

davidgrayston Dec 9, 2016

agentrickard Dec 12, 2016

davidgrayston Dec 12, 2016

davidgrayston Dec 12, 2016

davidgrayston Dec 13, 2016 •

edited

agentrickard Dec 13, 2016

davidgrayston Dec 13, 2016

agentrickard Dec 13, 2016

andypost Jan 9, 2017

davidgrayston Dec 9, 2016

davidgrayston Dec 9, 2016

davidgrayston Dec 9, 2016

davidgrayston Dec 10, 2016

davidgrayston Dec 10, 2016

davidgrayston commented Dec 10, 2016 •

edited

andypost commented Feb 13, 2017

dmiric commented Feb 25, 2017

dmiric commented Feb 25, 2017

andypost commented Feb 26, 2017

dmiric commented Feb 26, 2017 via email

andypost commented Mar 10, 2017

andypost commented Jan 28, 2018

agentrickard commented Jan 29, 2018

rivimey commented Jun 7, 2018

andypost commented Jun 7, 2018

rivimey commented Jun 11, 2018

agentrickard commented Jun 27, 2018

davidgrayston commented Jun 27, 2018

agentrickard commented Jun 27, 2018

8.x 1.x config ui domain switch #294

8.x 1.x config ui domain switch #294

Conversation

davidgrayston commented Nov 29, 2016

zerolab commented Nov 29, 2016 • edited

davidgrayston commented Dec 3, 2016

agentrickard commented Dec 6, 2016

agentrickard commented Dec 6, 2016

davidgrayston commented Dec 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgrayston Dec 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgrayston commented Dec 10, 2016 • edited

andypost commented Feb 13, 2017

dmiric commented Feb 25, 2017

dmiric commented Feb 25, 2017

andypost commented Feb 26, 2017

dmiric commented Feb 26, 2017 via email

andypost commented Mar 10, 2017

andypost commented Jan 28, 2018

agentrickard commented Jan 29, 2018

rivimey commented Jun 7, 2018

andypost commented Jun 7, 2018

rivimey commented Jun 11, 2018

agentrickard commented Jun 27, 2018

davidgrayston commented Jun 27, 2018

agentrickard commented Jun 27, 2018

zerolab commented Nov 29, 2016 •

edited

davidgrayston Dec 13, 2016 •

edited

davidgrayston commented Dec 10, 2016 •

edited