🇧🇪Belgium @kriboogh

kriboogh → created an issue.

The 3 "Only string literals should be passed to t() where possible (Drupal.Semantics.FunctionT.NotLiteralString)" errors will remain, as we can't write that code in any other form.

kriboogh → made their first commit to this issue’s fork.

Basic tests added, please review.

nah, go ahead ;-)
I'll close this one.

I'll closed this one, and start fresh for 11.x at 3450553, is gonna be easier then trying to rebase all this. work will continue there.

kriboogh → created an issue.

@kristiaanvandeneynde sure, it's not pretty though :D
We had to do some specific stuff with system.site config, which might have been only related to our project. But I'll keep it in.

Disclaimer: To anyone else reading this, this is specific project code given as an example, do not run this blindly on your production site !!!

function my_module_update_9000(&$sandbox) {

  // See patch https://www.drupal.org/project/drupal/issues/3150540.
  \Drupal::configFactory()->getEditable('locale.settings')->set('update_default_config_langcodes', FALSE)->save();

  // 1. Fix system.site config.
  $db = \Drupal::database();
  $db->delete('config')
    ->condition('collection', 'language.en')
    ->condition('name', 'system.site')
    ->execute();

  // Make sure the default config is set up as 'en'.
  $default_langcode = \Drupal::languageManager()->getDefaultLanguage()->getId();
  $default_config = \Drupal::configFactory()->getEditable('system.site');
  if ($default_config->get('langcode') == $default_langcode) {

    // Keep the default langcode name, we need it for the new override.
    $old_default_config_data = $default_config->getRawData();

    // Change the default langcode to english.
    $default_config->set('langcode', 'en');

    // Check if we have an english translation override, copy over its values.
    $config_en_translation = \Drupal::languageManager()->getLanguageConfigOverride('en', 'system.site');
    if (!$config_en_translation->isNew()) {
      $default_config->set('name', $config_en_translation->get('name'));
      $default_config->set('mail', $config_en_translation->get('mail'));
    }
    $default_config->save();

    // Create a new language override for the default langcode and store
    // the old values.
    if (!empty($old_default_config_data['name']) || !empty($old_default_config_data['mail'])) {
      $config_default_langcode_translation = \Drupal::languageManager()->getLanguageConfigOverride($default_langcode, 'system.site');
      if (!empty($old_default_config_data['name'])) {
        $config_default_langcode_translation->set('name', $old_default_config_data['name'] ?? '');
      }
      if (!empty($old_default_config_data['mail'])) {
        $config_default_langcode_translation->set('mail', $old_default_config_data['mail'] ?? '');
      }
      $config_default_langcode_translation->save();
    }
  }

  // Now cleanup the language overrides, they should only contain the site name and mail.
  foreach (\Drupal::languageManager()->getLanguages() as $langcode => $language) {
    if ($default_config->get('langcode') != $langcode) {
      // Only do overrides, skip the default config.
      $config_translation = \Drupal::languageManager()->getLanguageConfigOverride($langcode, 'system.site');
      if (!$config_translation->isNew()) {
        // Ignore non existing overrides.
        $config_translation->clear('page');
        $config_translation->save();
      }
    }
  }

  // 2. Check all default config langcodes and check if a language override
  // exists with the same langcode. Fix that.
  $query_string = <<<MYSQL
    SELECT c1.name, c1.collection
    FROM config c1, (SELECT c.name,
        REGEXP_REPLACE(REGEXP_SUBSTR(c.data, 's:8:"langcode";s:2:"(..)"'), '.*s:2:"(..)"', '\\\\1') as langcode
        FROM config c
        WHERE
          c.collection = ''
        AND
          c.data REGEXP 's:8:"langcode";s:2:".."'
        ) as clang
    WHERE
    c1.name = clang.name
    AND
    c1.collection = CONCAT('language.', clang.langcode)
  MYSQL;

  $rows = $db->query($query_string, [], ['allow_delimiter_in_query' => TRUE])->fetchAllAssoc('name');
  foreach ($rows as $row) {
    $db->delete('config')
      ->condition('collection', $row->collection)
      ->condition('name', $row->name)
      ->execute();
  }

  // 3. Swap default language config with language.en overrides.
  $query_string = <<<MYSQL
    SELECT c1.*
    FROM config c1, (SELECT c2.name FROM config c2 WHERE c2.collection = '' AND c2.data REGEXP 's:8:"langcode";s:2:"{$default_langcode}"') as c2
    WHERE c1.name = c2.name and c1.collection = 'language.en'
  MYSQL;

  $rows = $db->query($query_string, [], ['allow_delimiter_in_query' => TRUE])->fetchAllAssoc('name');
  foreach ($rows as $name => $row) {

    $default_config = \Drupal::configFactory()->getEditable($name);
    $english_config_override = \Drupal::languageManager()->getLanguageConfigOverride('en', $name);

    if (!$english_config_override->isNew() && !$default_config->isNew()) {
      // Only do this if both configs already exist!
      // Copy the english override into the default config and change it
      // to english.
      $english_config_override_data = $english_config_override->get();
      $default_config_data = $default_config->getRawData();
      $english_default_data = array_replace_recursive($default_config_data, $english_config_override_data);
      $default_config->setData($english_default_data);
      $default_config->set('langcode', 'en');
      $default_config->save();

      // Now take the original default config values and create a new
      // language override with the default language. Store only the keys
      // from the default config that where present in the english override.
      $default_config_override = \Drupal::languageManager()->getLanguageConfigOverride($default_langcode, $name);
      $default_config_override_data = array_intersect_key_recursive_values_2($english_config_override_data, $default_config_data);
      $default_config_override->setData($default_config_override_data);
      $default_config_override->save();

      // Delete the english override.
      $english_config_override->delete();
    }
  }

}

/**
 * Do a recursive key intersect but return values from array 2.
 *
 * @param array $array1
 * @param array $array2
 *
 * @return array
 */
function array_intersect_key_recursive_values_2(array $array1, array $array2) : array {

  $intersection = [];

  foreach ($array1 as $key => $value) {
    if (isset($array2[$key])) {
      if (is_array($array1[$key]) && is_array($array2[$key])) {
        $intersection[$key] = array_intersect_key_recursive_values_2($array1[$key], $array2[$key]);
      }
      else {
        $intersection[$key] = $array2[$key];
      }
    }
  }

  return($intersection);
}

I think that by allowing the config_langcode to be configurable like that, you're gonna end up in the same mess if someone changes that config will the site already has config in the first language.

By having a default config in EN always and have the language config overrides system (which already is in place and working) deal with the translations, everything should basically work out of the box (sort of). The only problem is "fixing" existing websites when we introduce this fixed EN config langcode.

We have done this in a current project. Basically you need to get all default config that was stored in the current default language (for example NL). Change the langcode of the default config to EN. WE now have a EN default config with NL data. So look if a EN config override exists. If there is, swap all data from the EN override into the new EN default. If there isn't an EN override, look for the original EN source strings of the data in the NL data and copy that into the new default EN config. To create a NL override, we can either let core create it (by looking up the translations through locale, or copy the old default translatable NL keys (labels, text,...) from the original NL default config into the new NL override.

Fix reported error

Added a basic test to see if the drupalSetting is set in javascript if supportsCredentials is set in config.

kriboogh → changed the visibility of the branch 3338518-cors-ajax-101 to hidden.

kriboogh → changed the visibility of the branch 3338518-cors-is-not to hidden.

Haven't experimented with other modules outside the "openai" ones. But I though to generalise it, since it's a public service you can use it directly, only then you will need to catch the exception yourself if you don't want a wsod. All code in this module should use the api class though. In that case we could then just deal with all of this in the api class and let other modules figure it out themselves if they decide to directly use the client. It's also a possible route to take of course.

Indeed, I first added the catches to the \Drupal\openai\OpenAIApi class, but the problem (if you can call it that) is that I noticed some code is calling the client service directly, not using the wrapping OpenAiApi class. So any code using the service directly would bypass the checks. By doing it in the Client, all code is covered. But as you mentioned, it creates a dependency on the openai php client class.

Just discovered that this patch actually needs the core patch mentioned in https://www.drupal.org/project/drupal/issues/2727667 →
This core patch solves the problem that when a batch is set in a submit handler for a form entity, the subsequent calls to entity save are not callable.

So the above merge request removes the event subscriber.

Instead, a wrapper is made around the OpenAI\Client, where each contract call is checked if the config is set or not. If not a new exception is thrown (ApiKeyNotConfiguredException). This exception is then catched by the OpenAiApi class, and logs the exception as before but also displays a messenger error if the user has permissions to configure the settings.
I also took the opportunity to use the interface (ClientContract) instead of the class for Client in the constructors. Same for the return types of the OpenAiAPi methods (they should return the Contract interfaces, not the classes).

I noticed some code instantiating the openai.client service directly instead of using the OpenAiApi service. Code doing that, should be responsible for dealing with exceptions in the contract calls itself. So this patch doesn't cover that (yet). Only if you call the OpenAiApi methods, the message will be shown.

There is also two helpers methods added, to prepareParameters and processResults for Contract calls being made.
This could in the future be used to send out events (for example OpenAiPrepareParametersEvent and OpenAiProcessResultsEvent) so other modules could hook into that. This is not yet implemented and perhaps is not in scope of this issue.

@gbyte, well actually we don't need the patch anymore, because I rewrote our code to deal with the queue differently.
(But it was the patch from @Ihnatsevych #4 that initially worked).

kriboogh → created an issue.

After re-reviewing this, we updated our UrlGenerator plugin to not store the actual entities in the queue items as this wasn't a good idea in the first place I think. We now just store the entity id in the queue item and load each entity on-demand when the queue item is processed.

We are also experiencing this issue with core Media classes being stored in the queue items.
The patch #4, works as a workaround.

I think the whole config translation system should be simplified and could be solved if:
- "default" config (the yml config files shipped with modules), is always in langcode "en" and contains English strings (obviously).
- All module translations are loaded and imported before the config is imported, so string translation is available.
- For each default English config, a language override is created when installing a site in a different language or when additional languages are added. So a site with languages NL and FR, with FR the default language, has English configs and two overridden configs in Dutch and French.
That way you can switch default site language whenever you want, as much as you want, add languages, delete them. Without default config ever needed to be updated.

This seems to be working for us. So if someone can review, maybe add some more tests?

kriboogh → created an issue.

Ah, yes, we are on the official release 8.x-1.1, that doesn't yet have the check you mentioned. But it is indeed the file does not exists warning.
But then again, why run code on file's that aren't PDF's anyway ?

kriboogh → created an issue.

Should be fixed with 1.1.3.

Thanks, couple of things based on the diff.

So working with a batch is great, but the way it is implemented, all users are still loaded in one call. We need to initialise a sandbox inside the batch context, load all user id's there using just a entity query on the user storage and then let batch iterate over them (either per user or in batches of lets say 100 users) until we are finished.

Setting up the batch can also be done using the BatchBuilder class.

The methods of the new class don't use dependency injection. So either solve that, or move the batch handling into the existing service and just add the dependency injection for the messenger service there.

#40 works for us.

I think config management in drupal should be that, all config is considered English always and if you install a different language, translatable keys in config are then dealt with using normal locale translation by default, or by a language config override if that's present.

kriboogh → created an issue.

I think you could just add it to the /core/rebuild.php file, and do a curl call to that instead.
So, modify rebuild.php like so:

  // Clear user cache for all major platforms.
  $user_caches = [
    'apcu_clear_cache',
    'wincache_ucache_clear',
  ];

Should become:

  // Clear user cache for all major platforms.
  $user_caches = [
    'apcu_clear_cache',
    'wincache_ucache_clear',
    'opcache_reset',
    'apcu_clear_cache',
  ];

Then drush cr can do a call to /rebuild.php.

Closing this, since there is no response anymore. If needed you can reopen with more details on the produced error.

kriboogh → created an issue.

kriboogh → created an issue.

kriboogh → created an issue.

kriboogh → created an issue.

kriboogh → created an issue.

kriboogh → created an issue.

kriboogh → created an issue.

Merged and fixed in 2.0.4

Merged and fixed in 2.0.4

Merged and fixed in 2.0.4

Merged and fixed in 2.0.4

Merged and fixed in 2.0.4

Merged and fixed in 2.0.4

Should be fixed, please review with later dev version.

kriboogh → made their first commit to this issue’s fork.

There is already a possibility to alter the url right before the redirect happens using:

function my_module_multi_domain_login_url(&$url, &$domain) {
  // Alter $url here.
} 

If you need more control, it is also possible to write a configuration override in your custom module and make it cache dependent on the user.

namespace Drupal\my_module\Config;

class MyModuleOverrides implements ConfigFactoryOverrideInterface {

  /**
   * {@inheritdoc}
   */
  public function loadOverrides($names) {
    $overrides = [];
    if (in_array("multi_domain_login.settings", $names)) {
      // Do custom stuff here to determine the $url.
      $overrides["multi_domain_login.settings"] = [
        'redirect_success' => $url,
      ];
    }

    return $overrides;
  }

 /**
   * {@inheritdoc}
   */
  public function getCacheableMetadata($name) {
    $cache = new CacheableMetadata();
    $cache->setCacheContexts(['user']);
    return $cache;
  }

If this could suit your needs, please close this ticket or reset to "needs work" than we will look into it.

Removed old code.

kriboogh → made their first commit to this issue’s fork.

kriboogh → made their first commit to this issue’s fork.

kriboogh → created an issue.

Module is D10 now.

Module is D10 now.

I'm on 10 too btw.

The reset button was still visible if a user had reorder permissions but no reset permissions. The merge request fixes this.
(Added patch of MR11 for secure use in composer).

Can confirm that #11 works. (user only having order terms permission now sees the table drag features).

kriboogh → created an issue.

Module is D10 ready.

I agree with @borisson_ on using Option A, putting declare on a separate line. Option B would create a mess if you would use multiple declare statements (in the future), which are part of PHP syntax. So besides a coding standard for just "declare(strict_types=1);" It would be better to look at the global picture and define a coding standard for using "declare" statements al together. (https://www.php.net/manual/en/control-structures.declare.php)

I think something went wrong here for the merge requests. :/

kriboogh → created an issue.

kriboogh → created an issue.

I fixed this in our case, by running a updb hook in one of our custom modules and including the editoria11y updb hooks (so no need to fiddle in the db).

function my_module_update_9000(&$sandbox) {
  Drupal::moduleHandler()->loadInclude("editoria11y", "install");
  editoria11y_update_9003($sandbox);
  editoria11y_update_9004($sandbox);
}

kriboogh → created an issue.

kriboogh → created an issue.