Added possibility to change the Site name per domain through the Administration page.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yivanov created an issue. See original summary.

yivanov’s picture

yivanov’s picture

Status: Patch (to be ported) » Active
agentrickard’s picture

This is not how I want the UI for config overrides to be handled. This should go through Domain Config and not be a property on the domain object.

There is already a project that takes this shortcut approach.

yivanov’s picture

Could you provide a link to that project that takes this approach?

About the rest - for me the Domain Config module looked more or less for general configuration (not per domain), that's why I took this approach at first place. (plus in the domain object you have the domain name, url scheme and status as part of it). But definitely I will think about your idea, since I want to be able to change also the domain theme and few other configs. Of course, it should be done the right way.

yivanov’s picture

FileSize
2.54 KB
agentrickard’s picture

Here it is -- https://www.drupal.org/project/domain_site_settings

The problem with Domain Config is that there is no easy way to build a UI for it. Config isn't naturally discoverable. So this module takes a shortcut and creates a custom configuration forms for some specific, common, items.

I don't like that approach, either, and have been hoping to come up with a better permanent solution.

agentrickard’s picture

I should add that I'm adhering to a pretty strict separation of concerns, which is why I want this feature handled by Domain Config.

robin.ingelbrecht’s picture

FileSize
11.6 KB

I looked into the domain site settings module and I don't think it's a good approach... It has it's own config overrider, it is not extendable, the settings aren't located on the domain, ...
With this in mind I have build a little UI extending on Domain Config to manage our domain dependable settings, I've been thinking about making this approach abstract so that all necessary config (including contrib config) can be domain dependable.

This is how I would approach it:
We provide a new annotation "DomainConfig" which includes ID, Label,Translatable and form elements. At this point we can define a plugin:

/**
 * Defines a domain config plugin.
 *
 * @DomainConfig(
 *   id = "system.site",
 *   label = @Translation("System site settings"),
 *   translatable = TRUE
 * )
 */
class DomainConfig Extends BaseDomainConfigPlugin {
  public function getFormElements() {
    return [
      'name' => [
        '#type'=> 'textfield',
        '#title'=> $this->t('Site name'),
        ...
      ],
      'email' => [
        ...
      ],
      'slogan' => [
        ...
      ],
    ];
  }
}

All plugins of type "DomainConfig" are then picked up by the UI and are displayed. In the submit callback of this new form all domain.config.DOMAIN_MACHINE_NAME.item.name config can be set.
Last but not least we can add all common core config as plugins in the Domain Config module.
This approach makes it very easy for other modules to expose their settings in the UI of domain config.

Any thoughts?
If we can agree on how we would implement this, I'm certainly willing to write patch!

yivanov’s picture

FileSize
3.38 KB

I've updated the patch with ability to change the domain theme from UI.
For everyone who wants it asap and can't wait for the stable module version of it.

robin.ingelbrecht’s picture

You clearly didn't read the comments of agentrickard... your patch is bad practice

agentrickard’s picture

@robin.ingelbrecht the original patch won't work, but the ideas in #9 are solid, assuming we can't auto-discover the configuration we need.

The idea of plugins for configuration-per-domain is the approach we took with D5 - D7. I don't particularly like doing it simply because it forces all the work on us. It might be the best short-term solution, and the plugin model is a strong approach.

Ideally, we should be able to discover all config entities and create forms for them. But I don't think we can at this point. See the notes in https://github.com/agentrickard/domain/issues/15 that discuss the challenge here, which is mirrored/summarized in #2677124: Allow discovery of configuration UI.

The approach in #9 would work, but might also face the problem reported in https://github.com/agentrickard/domain/issues/348.

See also the UI-overlay approach in https://github.com/agentrickard/domain/pull/294, which I still haven't had time to test.

robin.ingelbrecht’s picture

@agentrickard

I don't particularly like doing it simply because it forces all the work on us

If we use a plugin it doesn't though? Domain config should provide some core settings like site name, slogan, 404, .... But developers should expose their own settings by providing a plugin. This forces the work on the developer of the module (contrib or custom)

The approach in https://github.com/agentrickard/domain/pull/294 isn't ideal either imo. I prefer domain dependable settings bundled on the domain settings page (or a separate tab as in my example). Don't know what your opinion is?
Also, it exposes all admin pages to be saved as domain dependable settings which is unwanted behaviour. Not all settings have to be domain dependable no?

agentrickard’s picture

Correct.

Let's try the plugin approach for some core items and see where that takes us. And I wonder if it means we have to make a collection.

The fact of implementation is that most contrib modules don't implement the hooks/plugins in D7 and lower, so it generally falls on individual projects to implement.

In D7, we had an easier way to discover and exclude configuration forms. In D8, that only works for translatable strings.

agentrickard’s picture

One caveat on the plugins that bit us in earlier versions:

* Make sure the plugin class has a method for checking permissions. The plugin should only be available to people with permission to edit the related setting.
* Admins should only be able to change settings for domains that they are assigned to. See DomainAccessControlHandler::checkAccess() for how to enforce that check. I would consider changing settings to be an 'update' operation.

robin.ingelbrecht’s picture

I'll try and write a patch somewhere next week or do you prefer a pull request on github?

robin.ingelbrecht’s picture

A UI related thing:

I was thinking about making a table layout (like config entities) where each row contains a specific collection of settings. For example: a row for "system.site", a row for "user.settings", ... When clicking the "Edit" button on a row, the system redirects you to a new page where the form with all the corresponding settings is presented. What do you think?

agentrickard’s picture

That's what I had hoped to build using auto-discovery instead of a plugin system, so yes, that kind of UI makes sense to me.

robin.ingelbrecht’s picture

@agentrickard Making great progress on the patch. Almost ready for first review. But I'm stuck on one thing. How would you implement the domain language dependable settings UI wise? Something like Drupal core does: Add an extra tab "Translate" on the form?

andypost’s picture

btw It also works for each language so translation forms should care to get data from domain config overrides

#18 discovery is cachable but why not use separate config collection for overrides?

agentrickard’s picture

Because config collections are not documented as far as I know, so they are quite hard to create. I'm for using them if it makes sense, but when writing the original code, couldn't make any headway in using collections.

agentrickard’s picture

As to the UI, I would think that language is either a new tab, or, if possible, a selector at the top of the form, which would reduce clicking around.

agentrickard’s picture

Status: Active » Closed (outdated)