Add the domain config UI module

Interface added and reworked using last changes.

Thanks. Does this patch make any changes to what's in https://github.com/agentrickard/domain/pull/434? If so, it isn't immediately clear.

And what is the interdiff running against?

Note: what we really need now are tests. That's what's holding up this feature.

@agentrickard its rebase including latest changes from github, and some addition work (Interface isolated). Interdiff against previous patch.

I can't integrate this without a patch against the GutHub branch or isolation of the new work.

This new interface has been added, thanks to @gaydabura.

Probably easiest to pull patches from https://patch-diff.githubusercontent.com/raw/agentrickard/domain/pull/43...

Yes, I've been actively making changes over in GitHub.

The link in comment #6 should always have the latest changes.

https://patch-diff.githubusercontent.com/raw/agentrickard/domain/pull/43...

I just started to play around with this and I'll report back any issues I have.

What is needed to get this merged into the dev branch?

I've been on vacation. The missing piece is automated tests.

Need to reroll this patch according to latest dev version (mostly related to changes in .travis.yml)

Here's the latest version.

What I really need help with is conversion of the DomainConfigUIOverviewTest to support the Ajax load of the form element. It looks like we have to use FunctionJavascript for that.

See https://www.drupal.org/docs/8/phpunit/running-phpunit-javascript-tests

Latest patch looks great! just needs lots of clean-up

Most critical is
- request usage and passing from controllers to service methods
- session changes - stop using global and use from request

Remains are info file and DI clean-ups - not critical

1. +++ b/domain_config_ui/domain_config_ui.info.yml
   @@ -0,0 +1,9 @@
   +core: 8.x
   +core_version_requirement: ^8 || ^9
   

   it should be only one of them, better use core_version_requirement: ^8.7.7 || ^9
   https://www.drupal.org/node/3070687
   OTOH maybe it is not needed for submodules!

2. +++ b/domain_config_ui/domain_config_ui.module
   @@ -0,0 +1,150 @@
   +  if (!domain_config_ui_route_is_admin()) {
   ...
   +  if (!domain_config_ui_route_is_allowed()) {
   +    $content = ['domain_config_ui_admin' => domain_config_ui_admin_form('enable')];
   ...
   +  if ($content) {
   +    $variables['page']['content'] = array_merge($content, $variables['page']['content']);
   

   needs to explore if lazy builders can help here... also it could be packaged to controller

3. +++ b/domain_config_ui/domain_config_ui.module
   @@ -0,0 +1,150 @@
   +    $form = \Drupal::formBuilder()->getForm('Drupal\domain_config_ui\Form\SwitchForm');
   

   SwitchForm::class to keep usage

4. +++ b/domain_config_ui/domain_config_ui.module
   @@ -0,0 +1,150 @@
   +    $route = \Drupal::routeMatch()->getRouteObject();
   ...
   +          'route_name' => \Drupal::routeMatch()->getRouteName(),
   

   could use $route_match variable to call less \Drupal::

5. +++ b/domain_config_ui/domain_config_ui.services.yml
   @@ -0,0 +1,13 @@
   +    decoration_priority: 1
   

   could use comment why 1

6. +++ b/domain_config_ui/src/Config/ConfigFactory.php
   @@ -0,0 +1,164 @@
   +  public function __construct(StorageInterface $storage, EventDispatcherInterface $event_dispatcher, TypedConfigManagerInterface $typed_config, DomainConfigUIManager $domain_config_ui_manager) {
   +    parent::__construct($storage, $event_dispatcher, $typed_config);
   +    $this->domainConfigUIManager = $domain_config_ui_manager;
   ...
   +  public function setDomainConfigUiManager(DomainConfigUIManager $domain_config_ui_manager) {
   +    $this->domainConfigUIManager = $domain_config_ui_manager;
   

   It looks constructor could be removed for setter only - then less dependency on parent class

7. +++ b/domain_config_ui/src/Controller/DomainConfigUIController.php
   @@ -0,0 +1,269 @@
   +      \Drupal::messenger()->addMessage($this->t('The operation failed.'));
   ...
   +        'name' => t('Configuration key'),
   ...
   +    // @TODO: inject services.
   

   t() calls also TODO to clean-up

8. +++ b/domain_config_ui/src/DomainConfigUIManager.php
   @@ -0,0 +1,67 @@
   +  public function __construct(RequestStack $request_stack) {
   +    $this->request = $request_stack->getCurrentRequest();
   ...
   +    if ($domain = $this->request->get('domain_config_ui_domain')) {
   ...
   +    if (isset($_SESSION['domain_config_ui_domain'])) {
   +      return $_SESSION['domain_config_ui_domain'];
   ...
   +    if ($language = $this->request->get('domain_config_ui_language')) {
   ...
   +    if (isset($_SESSION['domain_config_ui_language'])) {
   +      return $_SESSION['domain_config_ui_language'];
   

   why request object serialized?
   Looks getSelectedConfigName() require $request argument to bypass from usage places

   Also sessions needs convert for 8.9 and 9.0 https://www.drupal.org/node/3109877

9. +++ b/domain_config_ui/src/DomainConfigUIManagerInterface.php
   @@ -0,0 +1,39 @@
   +interface DomainConfigUIManagerInterface {
   ...
   +  public function getSelectedConfigName($name, $omit_language = FALSE);
   ...
   +  public function getSelectedDomainId();
   ...
   +  public function getSelectedLanguageId();
   

   Maybe mark it internal for a while? to decide about session and request by-pass?

   Related #2473875: Convert uses of $_SESSION to symfony session retrieved from the request

10. +++ b/domain_config_ui/src/Form/DeleteForm.php
    @@ -0,0 +1,99 @@
    + * Class DeleteForm.
    ...
    +class DeleteForm extends FormBase {
    ...
    +    $form['actions']['#type'] = 'actions';
    +    $form['actions']['submit'] = [
    ...
    +    $form['actions']['cancel'] = [
    ...
    +    \Drupal::configFactory()->getEditable($name)->delete();
    

    Could extend ConfirmFormBase or ConfigFormBase

11. +++ b/domain_config_ui/src/Form/SettingsForm.php
    @@ -0,0 +1,85 @@
    +    unset($_SESSION['domain_config_ui_domain']);
    +    unset($_SESSION['domain_config_ui_language']);
    

    TBD convert sessions

12. +++ b/domain_config_ui/src/Form/SwitchForm.php
    @@ -0,0 +1,232 @@
    +  public function __construct(EntityTypeManagerInterface $entity_type_manager, LanguageManagerInterface $language_manager, DomainConfigUIManager $domain_config_ui_manager, DomainElementManagerInterface $domain_element_manager) {
    +    $this->domainConfigUiManager = $domain_config_ui_manager;
    +    $this->languageManager = $language_manager;
    +    $this->entityTypeManager = $entity_type_manager;
    +    $this->domainStorage = $this->entityTypeManager->getStorage('domain');
    +    $this->domainElementManager = $domain_element_manager;
    +    // Not loaded directly since it is not an interface.
    +    $this->accessHandler = $this->entityTypeManager->getAccessControlHandler('domain');
    

    missing logger and messenger services, or messenger added via setter?

13. +++ b/domain_config_ui/src/Form/SwitchForm.php
    @@ -0,0 +1,232 @@
    +      '#title' => 'Domain Configuration',
    

    missing $this->t()

14. +++ b/domain_config_ui/tests/src/Functional/DomainConfigUIOptionsTest.php
    @@ -0,0 +1,128 @@
    +  public static $modules = [
    
    +++ b/domain_config_ui/tests/src/Functional/DomainConfigUISettingsTest.php
    @@ -0,0 +1,95 @@
    +  public static $modules = [
    
    +++ b/domain_config_ui/tests/src/FunctionalJavascript/DomainConfigUIOverrideTest.php
    @@ -0,0 +1,153 @@
    +  public static $modules = [
    

    $modules property are protected

    Also it needs defaultTheme for 8.8 https://www.drupal.org/node/3083055

15. +++ b/domain_config_ui/tests/src/FunctionalJavascript/DomainConfigUIOverrideTest.php
    @@ -0,0 +1,153 @@
    +    sleep(3);
    

    it will bring flux to tests, the same for wait ajax in js-tests

16. +++ b/domain_config_ui/tests/src/Traits/DomainConfigUITestTrait.php
    @@ -0,0 +1,84 @@
    +  public function createAdminUser() {
    ...
    +  public function createEditorUser() {
    ...
    +  public function createLimitedUser() {
    ...
    +  public function createLanguageUser() {
    

    could use to return $this to use chaining in setUp()

I am totally NOT ready for that kind of critique.

My focus right now is test coverage.

Once tests are passing, we can worry about these issues.

It's not a critique, just review of current state from #d9readiness point of view

About tests - missing test coverage for manager and overrides - probably kernel test should be enough
The trick is to test variations of domain override and domain + language override

D9 readiness is a separate issue that comes after getting the code working.

We have variations of domain/language testing in Domain Config now. What we need coverage for is this new form interaction.

The challenge right now is that I can't get the UI to properly save the form without using JS tests -- and using JS tests, things fail that work using Functional tests.

Here's the latest patch. See Drupal\Tests\domain_config_ui\FunctionalJavaScript\DomainConfigUIOverrideTest

Maybe it will be enough to test whether the config changes get saved...

Good point! But checking anyway require to wait for click and then do check.

Yup, I made it work. The test could use a review.

Removes empty tests.

sigh.

Tests are passing locally (using Drupal 8.9-dev). Not sure what's going on with testbot.

Moved the failing test to FunctionalJavaScript and it works locally and on Travis.

New version with good test coverage.

Now it is ready for code review.

This has been testing just fine for me locally but failing on Travis and d.o., and I don't know why.

@agentrickard Thanks for this great contribution here. To enhance the code my first review is Coding Standards. To contribution I fixed all of this and this is a new patch with it.

interdiff patch 30 - 32

Right now, I am more concerned with figuring why the tests fail.

The first error looks like a environment is not setting to let "one" subdomain. Maybe is needed more work here. BTW I note that the tests works well at 1 April 2020 and now not. So they are several changes at tests that now the module domain tests needs maitenance.

1) Drupal\Tests\domain\Functional\Views\ActiveDomainDefaultArgumentTest::testActiveDomainDefaultArgument
GuzzleHttp\Exception\ConnectException: cURL error 6: Could not resolve host: one.php-apache-jenkins-drupal8-contrib-patches-31009

Yes, but there is no explanation for why this would start failing now. We haven't made any changes to the test setup.

What "changes at tests" are you referring to?

Hello,
installation breaks, on this line :

DomainConfigUIManager.php

if ($domain = $this->request->get('domain_config_ui_domain')) {
        return $domain;
}

$this->request is empty on the batch execution maybe.

another error :

Error: Call to a member function get() on null in Drupal\domain_config_ui\DomainConfigUIManager->getSelectedLanguageId() (line 62 of modules/contrib/domain/domain_config_ui/src/DomainConfigUIManager.php).

Same reason i think.

thx

How are you installing? I would expect to see this error when running tests.

[UPDATE]

I cannot replicate that report on a clean installation of Drupal 8.9, using the UI or Drush for enabling modules.

Please open a new issue with more information.

Respect to comment #38
Installation works fine. with drupal 8.8.6-dev + domain 8.x-1.x

Respect to comment #37
Currently the test Drupal\Tests\domain\Functional\Views\ActiveDomainDefaultArgumentTest::testActiveDomainDefaultArgument fail for branch 8.x-1.x too, soo it is not a problem only for this patch.
I recently created this issue here: 3134564

I Think that first is needed to fix it because fails at main branch contribution and it propage to all another issues.

The testing issue is a problem with configuration of the host, see #3133850: Subdomain registration has started failing. These tests all pass locally for me.

Trying the advice from #3133850: Subdomain registration has started failing

New patch.

One more time.

Try setting HOSTNAME directly.

@agentrickard what are you doing? can you up some explain what is the difference between patch to patch. BTW is a good practice to add a interdiff.

All changes are about related, to make tests to work in drupal-ci

I don't interdiff my own projects.

These patches are entirely because the tests pass fine locally, but started failing due to a change to the d.o. testbot.

At this point, I'm very frustrated by the testing. This is all working fine on local. I can't make Travis handle FunctionalJavaScript, and a change to d.o. testing broke our test runner.

I am considering two paths:

1) Commit this as is, and fix code-style and D9 issues separately.

2) Abandon this and leave it a patch only unless someone wants to support it as a standalone module.

And no one has an opinion? Has anyone tried running the tests locally?

Thank you!

I checked out the code from Github and played around with it on my local. It works great for our purposes, and I would love if it were able to be committed. A couple notes:

- I ran the Functional and FunctionalJavascript tests on my local on 8.8.4 and they all passed. I had attempted to run them on 8.7.1 and the Stark theme was not available, which broke one of the tests.
- On /admin/appearance, settings in the 'Administrative Theme' section are saved per domain, but setting a theme as default for the general site was always saved for all domains. This was confusing to me, and I think additional info should be present in the UI on that page. This could definitely be a follow-up issue.
- When I created an authenticated user as a Domain Administrator to one of my three domains, and gave the user the permission 'Manage domain-specific configurations', they were able to manage both their assigned domain and All Domains. Managing All Domains is a lot more power than I would expect that permission to have. This could also be a follow-up issue.

Thanks for that review.

I think we should take a look at item 3. If that is too much power, it's an error in the logic. Since we already have tests for things, it should be fairly easy to check.

So there is this note in the code: SwitchForm::getDomainOptions()

    // We always use 'all domains' here because it sets the default. Any user
    // who can access this form can set the default value.

Perhaps we should add an additional permission here.

The reason it's available here is that by default, you would have to have permission to set the setting without this module (e.g. Administer site settings).

Given that you may have multiple admins who should only set that value for a specific domain, adding a permission to 'set default site value' or similar seems wise.

Corrected an error with config loading in Drupal 8.9 and added a new permission 'set default domain configuration'.

Improved patch that sets the warning message string properly and updates the README.

@agentrickard with patch #7 and even patch #68, I'm getting an error in Drush since upgrading to Drush 10:

Error: Call to a member function get() on null in Drupal\domain_config_ui\DomainConfigUIManager->getSelectedDomainId() (line 47 of .../domain/domain_config_ui/src/DomainConfigUIManager.php)

Apparently, in Drush 10 $request_stack->getCurrentRequest() is null. To resolve this, I updated line 47 to:

if ($this->request && $domain = $this->request->get('domain_config_ui_domain')) {

and line 59 to:

if ($this->request && $language = $this->request->get('domain_config_ui_language')) {

I am so happy to see this! (had come across Domain Theme Switch and Domain Site Settings which OK for basic use cases but do not offer the same level of flexibility).

Really appreciate all this hard work and would be great to see this committed :-)

Have installed patch at #68 via composer and will report back any findings/feedback. I'll be extending site settings form across 20+ domains and using specific themes.

(I'm a comparative newbie contributing to issue threads or testing patches so feel free to offer any specific guidance on assisting constructively).

@pianomansam

What drush command are you running that triggers that error?

@agentrickard pretty much anything... even a drush status.

Hello, i'm having the same errors as #69, the proposed changes removes the errors (i have Drush Commandline Tool 9.7.2 )

But i'm having another issue, testing with css_vars module and a custom config form module, changing the domain reloads the page with the same exact variables and it's always "All domain" (its like nothing is happening)

variable is correctly passed in the url but the default option doesn't change ( please see the screen capture)

Thank you :)

Ok i have done some debugging, for me it seems to be a depedency injection problem, the request_stack doesn't contain the query params

Made some changes in getSelectedDomainId()

public function getSelectedDomainId() {
    $request_stack = \Drupal::service('request_stack');
    if ($domain = $request_stack->getCurrentRequest()->get('domain_config_ui_domain')) {
      return $domain;
    }

and getSelectedLanguageId :

/**
   * {@inheritdoc}
   */
  public function getSelectedLanguageId() {
    $request_stack = \Drupal::service('request_stack');

    if ($language = $request_stack->getCurrentRequest()->get('domain_config_ui_language')) {
      return $language;
    }

and everything works fine for me, no errors on drush and the domain change affects correctly the variables.

thx

@pianomansam and @hbensalem

What Drupal version are you running?

@agentrickard drupal 8.9

Thanks. Are there other domains in that dropdown on the form?

yes 4 in total

I'm also using Drupal 8.9 with Drush 10.

I cannot replicate that drush error. Perhaps it's an issue with drush launcher?

[success] Successfully enabled: domain_config_ui, domain_config
domain: drush status
 Drupal version   : 8.9.1-dev                                                       
 Site URI         : http://default                                                  
 DB driver        : mysql                                                           
 DB hostname      : localhost                                                       
 DB port          : 3306                                                            
 DB username      : test                                                            
 DB name          : domain8                                                         
 Database         : Connected                                                       
 Drupal bootstrap : Successful                                                      
 Default theme    : bartik                                                          
 Admin theme      : seven                                                           
 PHP binary       : /usr/bin/php                                                    
 PHP config       : /private/var/folders/kp/5qn2wy3163n8bqhx9cv1h25w0000gn/T/X2hlvn 
 PHP OS           : Darwin                                                          
 Drush script     : /usr/local/bin/drush                                            
 Drush version    : 10.2.2                                                          
 Drush temp       : /tmp                                                            
 Drush configs    : /Users/rickard/Sites/domain/vendor/drush/drush/drush.yml        
 Install profile  : standard                                                        
 Drupal root      : /Users/rickard/Sites/domain                                     
 Site path        : sites/default                                                   
 Files, Public    : sites/default/files                                             
 Files, Temp      : /tmp                                                            
domain: drush cr
 [success] Cache rebuild complete.
domain: drush domains
 --------------------- ---------- --------------------- -------- -------- --------- --------------- 
  Machine name          Name       Hostname              Scheme   Status   Default   HTTP Response  
 --------------------- ---------- --------------------- -------- -------- --------- --------------- 
  example_local         Domain 8   example.local         http     Active   Default   200 - OK       
  one_example_local     One        one.example.local     http     Active             200 - OK       
  two_example_local     Two        two.example.local     http     Active             200 - OK       
  three_example_local   Three      three.example.local   http     Active             200 - OK       
  four_example_local    Four       four.example.local    http     Active             200 - OK       
 --------------------- ---------- --------------------- -------- -------- --------- --------------- 
domain: 

These sorts of dependency injection failures and request interruptions typically happen when you have a module like Shield installed. Modules that implement the Middleware layer interfere with DI in very unpredictable ways.

I don't like the idea of removing dependency injection here, though we have to do similar things in Drupal\domain_config\DomainConfigOverrider::initiateContext()

Here's a new patch against the latest changes to 8.x-1.x.

Notes:

* To test, you must start from a clean install or rebuild the container (drush cr), since the service definition for `domain_config_ui.manager` has changed.
* It is possible that this will still throw the errors reported in #74, in which case we have to move the call out of the __construct() method and into getSelectedConfigName().

Hi @agentrickard

thx for the patch but still doesn't work for me, i can't start from a fresh install for testing, but i did what you suggested.

1/ apply patch, drush cr

PHP Fatal error:  Uncaught Error: Call to a member function get() on null in /app/web/modules/contrib/domain/domain_config_ui/src/DomainConfigUIManager.php:56
Stack trace:
#0 /app/web/modules/contrib/domain/domain_config_ui/src/Config/ConfigFactory.php(108): Drupal\domain_config_ui\DomainConfigUIManager->getSelectedDomainId()
#1 /app/web/core/lib/Drupal/Core/Config/ConfigFactory.php(89): Drupal\domain_config_ui\Config\ConfigFactory->doGet('search_api_atta...')
#2 /app/web/core/lib/Drupal.php(389): Drupal\Core\Config\ConfigFactory->get('search_api_atta...')
#3 /app/web/modules/contrib/search_api_attachments/search_api_attachments.module(29): Drupal::config('search_api_atta...')
#4 [internal function]: search_api_attachments_cache_flush()
#5 /app/web/core/lib/Drupal/Core/Extension/ModuleHandler.php(403): call_user_func_array('search_api_atta...', Array)
#6 /app/web/core/includes/common.inc(1033): Drupal\Core\Extension\ModuleHandler->invokeAll('cache_flush')
#7 /app/web/core/includes/utility.inc(55): drupal_flush_all_caches()
#8  in /app/web/modules/contrib/domain/domain_config_ui/src/DomainConfigUIManager.php on line 56

2/ comment getSelectedDomainId / getSelectedLanguageId() $this->request->get part
- drush cr

=> no errors

3/ put back code
- drush status

PHP Fatal error:  Uncaught Error: Call to a member function get() on null in /app/web/modules/contrib/domain/domain_config_ui/src/DomainConfigUIManager.php:56
Stack trace:
#0 /app/web/modules/contrib/domain/domain_config_ui/src/Config/ConfigFactory.php(108): Drupal\domain_config_ui\DomainConfigUIManager->getSelectedDomainId()
#1 /app/web/core/lib/Drupal/Core/Config/ConfigFactory.php(89): Drupal\domain_config_ui\Config\ConfigFactory->doGet('search_api_atta...')
#2 /app/web/core/lib/Drupal.php(389): Drupal\Core\Config\ConfigFactory->get('search_api_atta...')
#3 /app/web/modules/contrib/search_api_attachments/search_api_attachments.module(29): Drupal::config('search_api_atta...')
#4 [internal function]: search_api_attachments_cache_flush()
#5 /app/web/core/lib/Drupal/Core/Extension/ModuleHandler.php(403): call_user_func_array('search_api_atta...', Array)
#6 /app/web/core/includes/common.inc(1033): Drupal\Core\Extension\ModuleHandler->invokeAll('cache_flush')
#7 /app/web/core/includes/utility.inc(55): drupal_flush_all_caches()
#8  in /app/web/modules/contrib/domain/domain_config_ui/src/DomainConfigUIManager.php on line 56

thx

So these changes work for me ;

- put back the depedency injection. :

domain_config_ui.manager:
    class: Drupal\domain_config_ui\DomainConfigUIManager
    arguments: ['@request_stack']

change the constructor :

/**
   * Constructs a QueryHelper object.
   *
   * @param \Symfony\Component\HttpFoundation\RequestStack $requestStack
   *   The request stack.
   */
  public function __construct(RequestStack $requestStack) {
    $this->request = $requestStack;
  }

Change the functions :

/**
   * {@inheritdoc}
   */
  public function getSelectedDomainId() {
    if ($domain = $this->request->getCurrentRequest()->get('domain_config_ui_domain')) {
      return $domain;
    }
    if (isset($_SESSION['domain_config_ui_domain'])) {
      return $_SESSION['domain_config_ui_domain'];
    }
  }

  /**
   * {@inheritdoc}
   */
  public function getSelectedLanguageId() {
    if ($language = $this->request->getCurrentRequest()->get('domain_config_ui_language')) {
      return $language;
    }
    if (isset($_SESSION['domain_config_ui_language'])) {
      return $_SESSION['domain_config_ui_language'];
    }
  }

These sorts of dependency injection failures and request interruptions typically happen when you have a module like Shield installed. Modules that implement the Middleware layer interfere with DI in very unpredictable ways.

I don't have Shield installed nor do I have any modules that are modifying RequestStack AFAIK.

@hbensalam

So that would suggest that the RequestStack isn't always populated with currentRequest -- which is not something I can reproduce.

@pianomansam

Then your issue is probably with Drush Launcher not providing a url param. You may have to pass one manually.

@agentrickard only way that RequestStack can be populated is to put back the dependency .. i dont know how it works in you current setup

can you confirm that the proposed changes doesn't break the standard behavior ?

and like @pianomansam i dont have any module that are modifying the RequestStack.

thx.

Fixed up the Request / RequestStack load issue with some consistency. We do need to ensure that both lookups use the same request object,.

@agentrickard patch #88 works fine for me, thx ! ( ^____^ )

this module will be used on a live production site very soon, will keep you update ;)

Great! I'm still confused about why that happened, but I think the code is fine.

New patch now that d.o. testing is working again!

@pianomansam We will need to address that in a follow-up issue. I think this is ready for commit once we do a little code cleanup.

And a final revision with code style issues corrected. This patch also includes D9 compatibility.

One more cleanup.

One more time.

After a lot of work by a whole bunch of people, I have committed this patch.

I've upgraded to Beta 1 which includes this commit, and I am no longer seeing issues running commands in Drush 10.

Kudos, everybody.