From 09b18e5e5f54fad2ff10dfb75b386373d9b95268 Mon Sep 17 00:00:00 2001
From: catch <catch@35733.no-reply.drupal.org>
Date: Tue, 7 Nov 2023 16:26:38 +0000
Subject: [PATCH] Issue #3399295 by alexpott, Wim Leers, bircher, andypost,
borisson_: Allow all callables in ConfigTarget
---
core/lib/Drupal/Core/Form/ConfigFormBase.php | 26 +++--
core/lib/Drupal/Core/Form/ConfigTarget.php | 68 ++++++++---
.../jsonapi/src/Form/JsonApiSettingsForm.php | 11 +-
.../tests/src/Functional/SettingsFormTest.php | 8 +-
.../media/src/Form/MediaSettingsForm.php | 15 +--
.../Tests/Core/Form/ConfigTargetTest.php | 110 ++++++++++++++++++
6 files changed, 185 insertions(+), 53 deletions(-)
create mode 100644 core/tests/Drupal/Tests/Core/Form/ConfigTargetTest.php
diff --git a/core/lib/Drupal/Core/Form/ConfigFormBase.php b/core/lib/Drupal/Core/Form/ConfigFormBase.php
index 0bcfbe28981f..0d4460970c90 100644
--- a/core/lib/Drupal/Core/Form/ConfigFormBase.php
+++ b/core/lib/Drupal/Core/Form/ConfigFormBase.php
@@ -99,7 +99,7 @@ public function loadDefaultValuesFromConfig(array $element): array {
$value = $this->config($target->configName)->get($target->propertyPath);
if ($target->fromConfig) {
- $value = call_user_func($target->fromConfig, $value);
+ $value = ($target->fromConfig)($value);
}
$element['#default_value'] = $value;
}
@@ -134,11 +134,10 @@ public function storeConfigKeyToFormElementMap(array $element, FormStateInterfac
$map = $form_state->get(static::CONFIG_KEY_TO_FORM_ELEMENT_MAP) ?? [];
$target = $element['#config_target'];
- if (is_string($target)) {
- $target = ConfigTarget::fromString($target);
+ if ($target instanceof ConfigTarget) {
+ $target = $target->configName . ':' . $target->propertyPath;
}
- $target->elementParents = $element['#parents'];
- $map[$target->configName . ':' . $target->propertyPath] = $target;
+ $map[$target] = $element['#array_parents'];
$form_state->set(static::CONFIG_KEY_TO_FORM_ELEMENT_MAP, $map);
}
foreach (Element::children($element) as $key) {
@@ -158,7 +157,7 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
foreach ($this->getEditableConfigNames() as $config_name) {
$config = $this->config($config_name);
try {
- static::copyFormValuesToConfig($config, $form_state);
+ static::copyFormValuesToConfig($config, $form_state, $form);
}
catch (\BadMethodCallException $e) {
// Nothing to do: this config form does not yet use validation
@@ -193,7 +192,8 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
}
if (isset($map["$config_name:$property_path"])) {
- $form_element_name = implode('][', $map["$config_name:$property_path"]->elementParents);
+ $config_target = ConfigTarget::fromForm($map["$config_name:$property_path"], $form);
+ $form_element_name = implode('][', $config_target->elementParents);
}
else {
// We cannot determine where to place the violation. The only option
@@ -264,7 +264,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
foreach ($this->getEditableConfigNames() as $config_name) {
$config = $this->config($config_name);
try {
- static::copyFormValuesToConfig($config, $form_state);
+ static::copyFormValuesToConfig($config, $form_state, $form);
$config->save();
}
catch (\BadMethodCallException $e) {
@@ -287,10 +287,12 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
* The configuration being edited.
* @param \Drupal\Core\Form\FormStateInterface $form_state
* The current state of the form.
+ * @param array $form
+ * The form array.
*
* @see \Drupal\Core\Entity\EntityForm::copyFormValuesToEntity()
*/
- private static function copyFormValuesToConfig(Config $config, FormStateInterface $form_state): void {
+ private static function copyFormValuesToConfig(Config $config, FormStateInterface $form_state, array $form): void {
$map = $form_state->get(static::CONFIG_KEY_TO_FORM_ELEMENT_MAP);
// If there's no map of config keys to form elements, this form does not
// yet support config validation.
@@ -299,12 +301,12 @@ private static function copyFormValuesToConfig(Config $config, FormStateInterfac
throw new \BadMethodCallException();
}
- /** @var \Drupal\Core\Form\ConfigTarget $target */
- foreach ($map as $target) {
+ foreach ($map as $element_parents) {
+ $target = ConfigTarget::fromForm($element_parents, $form);
if ($target->configName === $config->getName()) {
$value = $form_state->getValue($target->elementParents);
if ($target->toConfig) {
- $value = call_user_func($target->toConfig, $value);
+ $value = ($target->toConfig)($value);
}
$config->set($target->propertyPath, $value);
}
diff --git a/core/lib/Drupal/Core/Form/ConfigTarget.php b/core/lib/Drupal/Core/Form/ConfigTarget.php
index ddf7bc500071..b7039a3c58c8 100644
--- a/core/lib/Drupal/Core/Form/ConfigTarget.php
+++ b/core/lib/Drupal/Core/Form/ConfigTarget.php
@@ -4,6 +4,8 @@
namespace Drupal\Core\Form;
+use Drupal\Component\Utility\NestedArray;
+
/**
* Represents the mapping of a config property to a form element.
*/
@@ -21,6 +23,20 @@ final class ConfigTarget {
*/
public array $elementParents;
+ /**
+ * Transforms a value loaded from config before it gets displayed by the form.
+ *
+ * @var \Closure|null
+ */
+ public readonly ?\Closure $fromConfig;
+
+ /**
+ * Transforms a value submitted by the form before it is set in the config.
+ *
+ * @var \Closure|null
+ */
+ public readonly ?\Closure $toConfig;
+
/**
* Constructs a ConfigTarget object.
*
@@ -29,11 +45,11 @@ final class ConfigTarget {
* `system.site`.
* @param string $propertyPath
* The property path being read or written, e.g., `page.front`.
- * @param string|null $fromConfig
+ * @param callable|null $fromConfig
* (optional) A callback which should transform the value loaded from
* config before it gets displayed by the form. If NULL, no transformation
* will be done. Defaults to NULL.
- * @param string|null $toConfig
+ * @param callable|null $toConfig
* (optional) A callback which should transform the value submitted by the
* form before it is set in the config object. If NULL, no transformation
* will be done. Defaults to NULL.
@@ -41,22 +57,11 @@ final class ConfigTarget {
public function __construct(
public readonly string $configName,
public readonly string $propertyPath,
- public readonly ?string $fromConfig = NULL,
- public readonly ?string $toConfig = NULL,
+ ?callable $fromConfig = NULL,
+ ?callable $toConfig = NULL,
) {
- // If they're passed at all, $fromConfig and $toConfig need to be string
- // callables in order to guarantee that this object can be serialized as
- // part of a larger form array. If these could be arrays, then they could be
- // in the form of [$object, 'method'], which would break serialization if
- // $object was not serializable. This is also why we don't type hint these
- // parameters as ?callable, since that would allow closures (which can't
- // be serialized).
- if ($fromConfig) {
- assert(is_callable($fromConfig));
- }
- if ($toConfig) {
- assert(is_callable($toConfig));
- }
+ $this->fromConfig = $fromConfig ? $fromConfig(...) : NULL;
+ $this->toConfig = $toConfig ? $toConfig(...) : NULL;
}
/**
@@ -83,4 +88,33 @@ public static function fromString(string $target, ?string $fromConfig = NULL, ?s
return new self($configName, $propertyPath, $fromConfig, $toConfig);
}
+ /**
+ * Gets the config target object for an element from a form array.
+ *
+ * @param array $array_parents
+ * The array to locate the element in the form.
+ * @param array $form
+ * The form array.
+ *
+ * @return self
+ * A ConfigTarget instance.
+ */
+ public static function fromForm(array $array_parents, array $form): self {
+ $element = NestedArray::getValue($form, $array_parents);
+ if (!isset($element['#config_target'])) {
+ throw new \LogicException('The form element [' . implode('][', $array_parents) . '] does not have the #config_target property set');
+ }
+ $target = $element['#config_target'];
+ if (is_string($target)) {
+ $target = ConfigTarget::fromString($target);
+ }
+ if (!$target instanceof ConfigTarget) {
+ throw new \LogicException('The form element [' . implode('][', $array_parents) . '] #config_target property is not a string or a ConfigTarget object');
+ }
+
+ // Add the element information to the config target object.
+ $target->elementParents = $element['#parents'];
+ return $target;
+ }
+
}
diff --git a/core/modules/jsonapi/src/Form/JsonApiSettingsForm.php b/core/modules/jsonapi/src/Form/JsonApiSettingsForm.php
index 978ee6fee06a..d47a9df1df4e 100644
--- a/core/modules/jsonapi/src/Form/JsonApiSettingsForm.php
+++ b/core/modules/jsonapi/src/Form/JsonApiSettingsForm.php
@@ -35,17 +35,16 @@ public function buildForm(array $form, FormStateInterface $form_state) {
'#type' => 'radios',
'#title' => $this->t('Allowed operations'),
'#options' => [
- 1 => $this->t('Accept only JSON:API read operations.'),
- 0 => $this->t('Accept all JSON:API create, read, update, and delete operations.'),
+ 'r' => $this->t('Accept only JSON:API read operations.'),
+ 'rw' => $this->t('Accept all JSON:API create, read, update, and delete operations.'),
],
'#config_target' => new ConfigTarget(
'jsonapi.settings',
'read_only',
- // Convert the value to an integer when displaying the config value in
- // the form.
- 'intval',
+ // Convert the bool config value to an expected string.
+ fn($value) => $value ? 'r' : 'rw',
// Convert the submitted value to a boolean before storing it in config.
- 'boolval',
+ fn($value) => $value === 'r',
),
'#description' => $this->t('Warning: Only enable all operations if the site requires it. <a href=":docs">Learn more about securing your site with JSON:API.</a>', [':docs' => 'https://www.drupal.org/docs/8/modules/jsonapi/security-considerations']),
];
diff --git a/core/modules/jsonapi/tests/src/Functional/SettingsFormTest.php b/core/modules/jsonapi/tests/src/Functional/SettingsFormTest.php
index 3a590e922da2..d9912630ddc8 100644
--- a/core/modules/jsonapi/tests/src/Functional/SettingsFormTest.php
+++ b/core/modules/jsonapi/tests/src/Functional/SettingsFormTest.php
@@ -29,15 +29,15 @@ public function testSettingsForm(): void {
$this->drupalGet('/admin/config/services/jsonapi');
$page = $this->getSession()->getPage();
- $page->selectFieldOption('read_only', 0);
+ $page->selectFieldOption('read_only', 'rw');
$page->pressButton('Save configuration');
$assert_session = $this->assertSession();
$assert_session->pageTextContains('The configuration options have been saved.');
- $assert_session->fieldValueEquals('read_only', 0);
+ $assert_session->fieldValueEquals('read_only', 'rw');
- $page->selectFieldOption('read_only', 1);
+ $page->selectFieldOption('read_only', 'r');
$page->pressButton('Save configuration');
- $assert_session->fieldValueEquals('read_only', '1');
+ $assert_session->fieldValueEquals('read_only', 'r');
$assert_session->pageTextContains('The configuration options have been saved.');
}
diff --git a/core/modules/media/src/Form/MediaSettingsForm.php b/core/modules/media/src/Form/MediaSettingsForm.php
index 0a6b97e98a4b..84c8a39bd32a 100644
--- a/core/modules/media/src/Form/MediaSettingsForm.php
+++ b/core/modules/media/src/Form/MediaSettingsForm.php
@@ -104,7 +104,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
'#title' => $this->t('iFrame domain'),
'#size' => 40,
'#maxlength' => 255,
- '#config_target' => new ConfigTarget('media.settings', 'iframe_domain', toConfig: static::class . '::nullIfEmptyString'),
+ '#config_target' => new ConfigTarget('media.settings', 'iframe_domain', toConfig: fn(?string $value) => $value ?: NULL),
'#description' => $this->t('Enter a different domain from which to serve oEmbed content, including the <em>http://</em> or <em>https://</em> prefix. This domain needs to point back to this site, or existing oEmbed content may not display correctly, or at all.'),
];
@@ -118,17 +118,4 @@ public function buildForm(array $form, FormStateInterface $form_state) {
return parent::buildForm($form, $form_state);
}
- /**
- * Converts an empty string to NULL.
- *
- * @param string|null $value
- * The value to transform.
- *
- * @return string|null
- * The given string, or NULL if it was empty.
- */
- public static function nullIfEmptyString(?string $value): ?string {
- return $value ?: NULL;
- }
-
}
diff --git a/core/tests/Drupal/Tests/Core/Form/ConfigTargetTest.php b/core/tests/Drupal/Tests/Core/Form/ConfigTargetTest.php
new file mode 100644
index 000000000000..14605daf7e1c
--- /dev/null
+++ b/core/tests/Drupal/Tests/Core/Form/ConfigTargetTest.php
@@ -0,0 +1,110 @@
+<?php
+
+namespace Drupal\Tests\Core\Form;
+
+use Drupal\Core\Form\ConfigTarget;
+use Drupal\Tests\UnitTestCase;
+
+/**
+ * @coversDefaultClass \Drupal\Core\Form\ConfigTarget
+ * @group Form
+ */
+class ConfigTargetTest extends UnitTestCase {
+
+ /**
+ * @covers ::fromForm
+ * @covers ::fromString
+ */
+ public function testFromFormString() {
+ $form = [
+ 'group' => [
+ '#type' => 'details',
+ 'test' => [
+ '#type' => 'text',
+ '#default_value' => 'A test',
+ '#config_target' => 'system.site:name',
+ '#name' => 'test',
+ '#parents' => ['test'],
+ ],
+ ],
+ ];
+ $config_target = ConfigTarget::fromForm(['group', 'test'], $form);
+ $this->assertSame('system.site', $config_target->configName);
+ $this->assertSame('name', $config_target->propertyPath);
+ $this->assertSame(['test'], $config_target->elementParents);
+ }
+
+ /**
+ * @covers ::fromForm
+ */
+ public function testFromFormConfigTarget() {
+ $form = [
+ 'test' => [
+ '#type' => 'text',
+ '#default_value' => 'A test',
+ '#config_target' => new ConfigTarget('system.site', 'admin_compact_mode', 'intval', 'boolval'),
+ '#name' => 'test',
+ '#parents' => ['test'],
+ ],
+ ];
+ $config_target = ConfigTarget::fromForm(['test'], $form);
+ $this->assertSame('system.site', $config_target->configName);
+ $this->assertSame('admin_compact_mode', $config_target->propertyPath);
+ $this->assertSame(['test'], $config_target->elementParents);
+ $this->assertSame(1, ($config_target->fromConfig)(TRUE));
+ $this->assertFalse(($config_target->toConfig)('0'));
+ }
+
+ /**
+ * @covers ::fromForm
+ * @dataProvider providerTestFromFormException
+ */
+ public function testFromFormException(array $form, array $array_parents, string $exception_message) {
+ $this->expectException(\LogicException::class);
+ $this->expectExceptionMessage($exception_message);
+ ConfigTarget::fromForm($array_parents, $form);
+ }
+
+ public function providerTestFromFormException(): array {
+ return [
+ 'No #config_target' => [
+ [
+ 'test' => [
+ '#type' => 'text',
+ '#default_value' => 'A test',
+ ],
+ ],
+ ['test'],
+ 'The form element [test] does not have the #config_target property set',
+ ],
+ 'No #config_target nested' => [
+ [
+ 'group' => [
+ '#type' => 'details',
+ 'test' => [
+ '#type' => 'text',
+ '#default_value' => 'A test',
+ ],
+ ],
+ ],
+ ['group', 'test'],
+ 'The form element [group][test] does not have the #config_target property set',
+ ],
+ 'Boolean #config_target nested' => [
+ [
+ 'group' => [
+ '#type' => 'details',
+ 'test' => [
+ '#type' => 'text',
+ '#config_target' => FALSE,
+ '#default_value' => 'A test',
+ ],
+ ],
+ ],
+ ['group', 'test'],
+ 'The form element [group][test] #config_target property is not a string or a ConfigTarget object',
+ ],
+ ];
+ }
+
+}
--
GitLab