Allow to load configuration override across all domains.

Created on 20 December 2023, about 1 year ago
Updated 2 July 2024, 6 months ago

Issue / Need

I am doing a migration deriver, which creates a derivative for each active domain, and uses the default configuration, or the domain overriden one if there is one. What i am doing now, is creating manually the configuration id for each domain, and then try to load them individually.

Proposed change / improvement

The "domain.config.overrider" could have a method to be able to load configuration overrides across all domains, not only the active one. Also, it would also be useful to change the method getDomainConfigMethod to public.

✨ Feature request
Status

Needs work

Version

2.0

Component

- Domain Config

Created by

πŸ‡ͺπŸ‡ΈSpain guiu.rocafort.ferrer Barcelona

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @guiu.rocafort.ferrer
  • Status changed to Needs review 12 months ago
  • πŸ‡ͺπŸ‡ΈSpain guiu.rocafort.ferrer Barcelona

    Change the status to needs review.

  • πŸ‡ΊπŸ‡ΈUnited States agentrickard Georgia (US)

    I wonder about how we might do this. We do something similar with languages.

    * If a language/domain specific config exists, use it.
    * Else use the domain config for all languages under that domain.

    I wonder if we could just allow prefixes to ignore the domain id portion of the config.

      protected function getDomainConfigName($name, DomainInterface $domain) {
        return [
          'langcode' => 'domain.config.' . $domain->id() . '.' . $this->language->getId() . '.' . $name,
          'domain' => 'domain.config.' . $domain->id() . '.' . $name,
          'langcode-global' => 'domain.config.' . $this->language->getId() . '.' . $name,
          'global' => 'domain.config.' . $name,
        ];
      }
    

    And then we would cascade through in that order, where now we just loop through the first two.

  • πŸ‡ͺπŸ‡ΈSpain guiu.rocafort.ferrer Barcelona

    "I wonder if we could just allow prefixes to ignore the domain id portion of the config". I don't think this is possible.

    As i see it, the fact that the domain and the language come before the configuration name makes it a bit difficult to retrieve a particular configuration name for all domains. If the configuration names had the format domain.config.{name}.{domain}.{langcode}, then it could be possible to load all overrides for a configuration using the prefix domain.config.{name}, but i guess this might be a big change in the domain_config module...

    In this commit ( https://git.drupalcode.org/issue/domain-3409936/-/commit/c82f391bea0e657... ) i added a method that retrieves all domains, and checks the overrides for a particular configuration in all of them. I expanded the method functionality to include language overrides in this one ( https://git.drupalcode.org/issue/domain-3409936/-/commit/341c4227254c684... ).

    I don't see how this could be done without loading and iterating over the domains, and in each one, iterating through the languages.

  • Status changed to Active 12 months ago
  • πŸ‡ΊπŸ‡ΈUnited States agentrickard Georgia (US)
    I don't see how this could be done without loading and iterating over the domains, and in each one, iterating through the languages.
    

    We do this by loading the config name first and then checking to see if a matching config file exists. That is, we loop through config names, not domains. So it sounds like what you are looking to do is counter to that logic.

    But I am struggling to understand the use-case here.

    You want to find all the overrides for each domain -- not at runtime, but during a migration action?

    I suspect that what you are doing is a special case that is better handled in the migration code (through a custom service) rather than ported back to the module.

    My advice is to use a service decorator to extend / alter the domain_config.overrider service to do what you need within the context of your migration.

    Otherwise, we should propose an entirely new service that we could call in order to do this type of operation, because what you are doing -- if I understand correctly -- is not what this service is built for.

    In an ideal world, we would use Config Collections ( #3060758: Investigate using config collections β†’ ), but that is a larger task, given that the collection system is largely undocumented.

  • πŸ‡ͺπŸ‡ΈSpain guiu.rocafort.ferrer Barcelona

    I think i didn't set my point properly, so i will try again:

    Generic issue: I have a specific configuration X. I want to know all the domains that override that specific configuration.

    Particular use case: In this specific example i am exposing, i have a configuration that stores some credentials to an external rest api, which i use to import the contents via a migration. Each domain has a different endpoint url, and different credentials. so, i use a deriver to automatically generate a different migration for each domain that has this configuration overriden.

    Potential use cases: I can think of some use cases for this, for example cron tasks that perform different actions according to each defined domain overriden configuration.

    What i am doing now: I am loading all the domains, and iterating over them, then i call getDomainConfigName for each one, and then check if that configuration name is present or not.

    I hope this time i was able to define more clearly the scope of the issue.

  • πŸ‡ΊπŸ‡ΈUnited States agentrickard Georgia (US)

    Generic issue:
    I have a specific configuration X. I want to know all the domains that override that specific configuration.

    Yes, this is great and very clear.

    I still think that this request is totally separate from what the domain_config.overrider service is for, so this patch is not valid as written.

    The overrider service is contextual to the current request in a way that your external migration is not. So we would need a new service that would allow that lookup to be more efficient. In versions < Drupal 8. we did something like this inside hook_cron() to account for such use-cases, but I don't think that would work here.

    The "best" way to do that is likely to refactor to use config collections, which are supposed to register overrides similar to your use-case, but I don't have time for that.

    For the short term, I can see a domain.config.collection service to run this calculation for you, then cache the output for efficiency.

  • πŸ‡ͺπŸ‡ΈSpain guiu.rocafort.ferrer Barcelona

    Thanks for the clarification,

    I failed to see that this service is not the right place to add this functionality due to the tight coupling with the DomainNegotiator.
    Because the use of config collections issue is going to take some time, i agree with adding a new service "domain.config.collection" for this. Later, this service can be reused to perform some operations with the config collections once that is in place.

    I will find some time to do this at some point today.

  • Status changed to Needs review 12 months ago
  • πŸ‡ͺπŸ‡ΈSpain guiu.rocafort.ferrer Barcelona

    I added a new service domain_config.collection ( to be coherent with the module name and the rest of the services ), and added the function there. To avoid repeating code, i made the getDomainConfigName method static, so it can be used from the new service without duplicating the code. I also added a Functional test for the service method.

    For some reason i cannot create a merge request for the branch i created, not sure what i should do about that...

  • πŸ‡ͺπŸ‡ΈSpain guiu.rocafort.ferrer Barcelona

    Changing the issue status to Major as per issue priorities β†’

  • Issue was unassigned.
  • πŸ‡ͺπŸ‡ΈSpain guiu.rocafort.ferrer Barcelona

    Changing the issue status to Crital as per issue priorities β†’

  • Status changed to Needs work 6 months ago
  • πŸ‡ΊπŸ‡ΈUnited States agentrickard Georgia (US)

    MR is out of date.

  • Pipeline finished with Success
    6 months ago
    Total: 211s
    #213290
  • Status changed to Needs review 6 months ago
  • πŸ‡ͺπŸ‡ΈSpain guiu.rocafort.ferrer Barcelona

    MR have been updated.

  • Status changed to Needs work 6 months ago
  • πŸ‡ΊπŸ‡ΈUnited States agentrickard Georgia (US)

    Nitpick: The comment "Return config overrides across all ( or ony active ) domains." has unwanted spacing around the ().

    Question:

    Adding the $language as an argument to getDomainConfigName() is an API change -- though so it making it public and static.

    Perhaps we should make a public static wrapper that uses the new arguments without editing the existing method? At the least $language should = NULL by default?

    A good test for the approach would be to rewrite the now redundant code in Drupal\domain_config_ui\Config\Config, which does this:

      /**
       * Get the domain config name.
       */
      protected function getDomainConfigName() {
        // Return selected config name.
        return $this->domainConfigUIManager->getSelectedConfigName($this->name);
      }
    

    And that calls Drupal\domain_config_ui\DomainConfigUIManager::getSelectedConfigName

      /**
       * {@inheritdoc}
       */
      public function getSelectedConfigName($name, $omit_language = FALSE) {
        if ($domain_id = $this->getSelectedDomainId()) {
          $prefix = "domain.config.{$domain_id}.";
          if (!$omit_language && $langcode = $this->getSelectedLanguageId()) {
            $prefix .= "{$langcode}.";
          }
          return $prefix . $name;
        }
        return $name;
      }
    

    Though that may not be worth it, since the DomainConfigUIManager uses ids and not objects.

Production build 0.71.5 2024