Cast TranslationWrapper objects in config to string during Config::set()/setData()

Nice find debugging! Added that same pattern to StorableConfigBase which solve most if not all of the remaining failures.

Actually I think they should do the cast to string on the $value instead of in the loop on the nested value that way the string on a simple key:value will also be cast appropriately. Does that make sense?

Rename the issue seems fine to me. That last fail seems tricky. It's going into the symfony yaml::dump(). Trying to breakpoint the test

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/BooleanItem.php
@@ -33,8 +34,8 @@ class BooleanItem extends FieldItemBase implements OptionsProviderInterface {
+      'on_label' => new TranslationWrapper('On'),
+      'off_label' => new TranslationWrapper('Off'),

IMHO we should cast it to string before we pass it to config entries ... in order to keep the existing behaviour, IMHO.

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -209,18 +209,17 @@ public function save($has_trusted_data = FALSE) {
-    if (!$has_trusted_data) {
-      if ($this->typedConfigManager->hasConfigSchema($this->name)) {
...
+    if (!$has_trusted_data && $this->typedConfigManager->hasConfigSchema($this->name)) {
...
+    else {
+      // We fall back to simple validation.
+      foreach ($this->data as $key => $value) {
+        $this->data[$key] = $this->validateValue($key, $value);
       }

I don't know what has_trusted_data but before it would never do the simple validation on trusted data and now it does because of the moved condition. Is this change needed?

I like how the simplification has come along on this, it looks way better!

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -209,18 +209,19 @@ public function save($has_trusted_data = FALSE) {
+    if (!$has_trusted_data && $this->typedConfigManager->hasConfigSchema($this->name)) {
+      // Ensure that the schema wrapper has the latest data.

It was exactly that what we wanted to optimize.

Would it not be enough to just use in the original else:

foreach ($this->data as $key => $value) {
        if ($value instanceof SafeStringInterface) {
          $this->data[$key] = (string) $value;
        }
}

I can profile the different options if someone has an idea of a good use case where that could be tested.

1. +++ b/core/lib/Drupal/Core/Config/Config.php
   @@ -209,18 +209,19 @@ public function save($has_trusted_data = FALSE) {
   -    if (!$has_trusted_data) {
   -      if ($this->typedConfigManager->hasConfigSchema($this->name)) {
   

   I'm not sure about this change.

2. +++ b/core/lib/Drupal/Core/Config/Config.php
   @@ -209,18 +209,19 @@ public function save($has_trusted_data = FALSE) {
   -    if (!$has_trusted_data) {
   -      if ($this->typedConfigManager->hasConfigSchema($this->name)) {
   -        // Ensure that the schema wrapper has the latest data.
   -        $this->schemaWrapper = NULL;
   -        foreach ($this->data as $key => $value) {
   -          $this->data[$key] = $this->castValue($key, $value);
   -        }
   +    if (!$has_trusted_data && $this->typedConfigManager->hasConfigSchema($this->name)) {
   +      // Ensure that the schema wrapper has the latest data.
   +      $this->schemaWrapper = NULL;
   +      foreach ($this->data as $key => $value) {
   +        $this->data[$key] = $this->castValue($key, $value);
          }
   -      else {
   -        foreach ($this->data as $key => $value) {
   -          $this->validateValue($key, $value);
   -        }
   +    }
   +    else {
   +      // We fall back to simple validation. Even for trusted data we still want
   +      // to do this, as TranslationWrappers may exist in the configuration
   +      // object which we will need to cast to string.
   +      foreach ($this->data as $key => $value) {
   +        $this->data[$key] = $this->validateValue($key, $value);
          }
        }
   

   I don't like this change for an number of reasons:

   1. Validating config with !$has_trust_data feels wrong - changing that data feels even more wrong.
   2. Having a method called validateValue actually result in a data change is bad.

   There's been a lot of tension throughout the D8 cycle about putting too much logic inside the Config object and its storage classes - personally I think this is a step in the wrong direction for the reasons given above.

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/BooleanItem.php
   @@ -33,8 +34,8 @@ class BooleanItem extends FieldItemBase implements OptionsProviderInterface {
   -      'on_label' => t('On'),
   -      'off_label' => t('Off'),
   +      'on_label' => new TranslationWrapper('On'),
   +      'off_label' => new TranslationWrapper('Off'),
        ) + parent::defaultFieldSettings();
   

   If these are the only translated things that end up in config why don't we just cast to string here.

I think we should cast on set. This means that $config->set('foo', t('bar')); $config->get('foo'); will return the string 'bar' rather than a safe string. To do this I've swapped some logic around in initWithData so we don't cast everything unnecessary when loading config from storage.

I didn't post an interdiff with #40 cause it would have been bigger than the patch.

Fixed nits from #42.

Well MigrateDrupal6Test runs all the tests including MigrateUserConfigsTest and MigrateUserTest so it's not surprising it has failed too.

I've run MigrateUserConfigsTest 100 times locally - no fails whatsoever and I can't see how this is introduced by this patch.

RTBC, looks great to me.

We'll need a CR though.

Committed/pushed to 8.0.x thanks!

I know this is closed, but right now I'm getting the same install error:

> PHP message: Uncaught PHP Exception Symfony\Component\Routing\Exception\RouteNotFoundException: "Route "search.view_node_search" does not exist." at core/lib/Drupal/Core/Routing/RouteProvider.php line 191

@jackbravo that doesn't look like this error at all. I couldn't find references to that route except in tests. Are you sure you have the latest 8.0.1 or dev release? If so try to run /core/rebuild.php or drush cr to rebuild the container.

Sorry, it probably isn't related. I saw the same error on some tests running for this issue (patch 15): https://www.drupal.org/pift-ci-job/27901

But probably it was because of something else because the next patch did not generate that error (patch 16): https://www.drupal.org/pift-ci-job/27903