Skip to content

Commit

Permalink
merged pull request #60 from craue/fix-form
Browse files Browse the repository at this point in the history
fixed issues with unintentional value changes when using the built-in form
  • Loading branch information
craue committed Aug 6, 2023
2 parents 5bf3673 + 090dae3 commit bdc253e
Show file tree
Hide file tree
Showing 10 changed files with 240 additions and 37 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
Expand Up @@ -28,7 +28,7 @@ jobs:
php: '7.3'
DEPS: 'lowest'
WITH_DOCTRINE_CACHE_BUNDLE: 'yes'
SYMFONY_DEPRECATIONS_HELPER: 'max[self]=2&max[indirect]=1230'
SYMFONY_DEPRECATIONS_HELPER: 'max[self]=2&max[indirect]=1394'
-
php: '8.2'
DEPS: 'unmodified'
Expand Down
2 changes: 1 addition & 1 deletion Controller/SettingsController.php
Expand Up @@ -37,10 +37,10 @@ public function modifyAction(CacheAdapterInterface $cache, FormFactoryInterface
$form->handleRequest($request);

if ($form->isSubmitted() && $form->isValid()) {
// update the cache
foreach ($formData['settings'] as $formSetting) {
$storedSetting = $this->getSettingByName($allStoredSettings, $formSetting->getName());
if ($storedSetting !== null) {
$storedSetting->setValue($formSetting->getValue());
$cache->set($storedSetting->getName(), $storedSetting->getValue());
}
}
Expand Down
16 changes: 12 additions & 4 deletions Form/ModifySettingsForm.php
Expand Up @@ -2,9 +2,10 @@

namespace Craue\ConfigBundle\Form;

use Craue\ConfigBundle\Entity\SettingInterface;
use Craue\ConfigBundle\Form\Type\SettingType;
use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\Extension\Core\Type\CollectionType;
use Symfony\Component\Form\Extension\Core\Type\FormType;
use Symfony\Component\Form\FormBuilderInterface;

/**
Expand All @@ -18,9 +19,16 @@ class ModifySettingsForm extends AbstractType {
* {@inheritDoc}
*/
public function buildForm(FormBuilderInterface $builder, array $options) : void {
$builder->add('settings', CollectionType::class, [
'entry_type' => SettingType::class,
]);
$settingsForm = $builder->create('settings', FormType::class);

foreach ($options['data']['settings'] as $setting) {
/* @var $setting SettingInterface */
$settingsForm->add($setting->getName(), SettingType::class, [
'data' => $setting,
]);
}

$builder->add($settingsForm);
}

/**
Expand Down
17 changes: 17 additions & 0 deletions Form/Type/SettingType.php
Expand Up @@ -5,6 +5,8 @@
use Craue\ConfigBundle\Entity\SettingInterface;
use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\Form\FormEvent;
use Symfony\Component\Form\FormEvents;
use Symfony\Component\Form\FormInterface;
use Symfony\Component\Form\FormView;
use Symfony\Component\OptionsResolver\OptionsResolver;
Expand Down Expand Up @@ -33,6 +35,21 @@ public function buildForm(FormBuilderInterface $builder, array $options) : void
'required' => false,
'translation_domain' => 'CraueConfigBundle',
]);

$builder->addEventListener(FormEvents::PRE_SUBMIT, function(FormEvent $event) : void {
$form = $event->getForm();
$submittedData = $event->getData();

// replace non-submitted values by defaults - this avoids nulling values of settings missing from the request
// idea from https://stackoverflow.com/questions/11687760/form-avoid-setting-null-to-non-submitted-field/16522446#16522446
foreach ($form->all() as $name => $child) {
if (!isset($submittedData[$name])) {
$submittedData[$name] = $child->getData();
}
}

$event->setData($submittedData);
});
}

/**
Expand Down
137 changes: 107 additions & 30 deletions Tests/Controller/SettingsControllerIntegrationTest.php
Expand Up @@ -3,6 +3,8 @@
namespace Craue\ConfigBundle\Tests\Controller;

use Craue\ConfigBundle\Entity\Setting;
use Craue\ConfigBundle\Entity\SettingInterface;
use Craue\ConfigBundle\Tests\IntegrationTestBundle\Entity\CanBeDisabledSetting;
use Craue\ConfigBundle\Tests\IntegrationTestBundle\Entity\CustomSetting;
use Craue\ConfigBundle\Tests\IntegrationTestCase;

Expand Down Expand Up @@ -39,8 +41,8 @@ public function testModifyAction_noChanges($platform, $config, $requiredExtensio
$content = static::$client->getResponse()->getContent();
$this->assertSame(200, static::$client->getResponse()->getStatusCode(), $content);
$this->assertMatchesRegularExpression('/<form .*method="post" .*class="craue_config_settings_modify".*>/', $content);
$this->assertStringContainsString('<label for="craue_config_modifySettings_settings_0_value">name</label>', $content);
$this->assertStringContainsString('<input type="text" id="craue_config_modifySettings_settings_0_value" name="craue_config_modifySettings[settings][0][value]" value="value" />', $content);
$this->assertStringContainsString('<label for="craue_config_modifySettings_settings_name_value">name</label>', $content);
$this->assertStringContainsString('<input type="text" id="craue_config_modifySettings_settings_name_value" name="craue_config_modifySettings[settings][name][value]" value="value" />', $content);
$this->assertStringContainsString('<button type="submit">apply</button>', $content);

$form = $crawler->selectButton('apply')->form();
Expand All @@ -49,15 +51,38 @@ public function testModifyAction_noChanges($platform, $config, $requiredExtensio
$content = static::$client->getResponse()->getContent();
$this->assertStringContainsString('<div class="notice">The settings were changed.</div>', $content);

$settings = $this->getSettingsRepo()->findAll();
$this->assertCount(1, $settings);

$setting = $settings[0];
/* @var $setting SettingInterface */
$setting = $this->getSettingsRepo()->findOneBy([]);
$this->assertSame('name', $setting->getName());
$this->assertSame('value', $setting->getValue());
$this->assertNull($setting->getSection());
}

/**
* Ensure that the value of a setting added between rendering and submitting the form won't get lost.
*
* @dataProvider getPlatformConfigs
*/
public function testModifyAction_noChanges_concurrentlyAddedSetting($platform, $config, $requiredExtension) : void {
$this->initClient($requiredExtension, ['environment' => $platform, 'config' => $config]);
$this->persistSetting(Setting::create('name1', 'value1'));
$this->persistSetting(Setting::create('name2', 'value2'));

$crawler = static::$client->request('GET', $this->url('craue_config_settings_modify'));
$form = $crawler->selectButton('apply')->form();

// add a new setting which would be placed between name1 and name2
$this->persistSetting(Setting::create('name11', 'value11'));

static::$client->submit($form);

/* @var $setting SettingInterface */
$setting = $this->getSettingsRepo()->findOneBy([
'name' => 'name11',
]);
$this->assertSame('value11', $setting->getValue());
}

/**
* Ensure that only the value can be changed, but neither name nor section.
*
Expand All @@ -69,19 +94,17 @@ public function testModifyAction_changeValue($platform, $config, $requiredExtens

$crawler = static::$client->request('GET', $this->url('craue_config_settings_modify'));
$form = $crawler->selectButton('apply')->form();
$this->assertFalse($form->has('craue_config_modifySettings[settings][0][name]'));
$this->assertFalse($form->has('craue_config_modifySettings[settings][0][section]'));
$this->assertFalse($form->has('craue_config_modifySettings[settings][name][name]'));
$this->assertFalse($form->has('craue_config_modifySettings[settings][name][section]'));
static::$client->followRedirects();
static::$client->submit($form, [
'craue_config_modifySettings[settings][0][value]' => 'new-value',
'craue_config_modifySettings[settings][name][value]' => 'new-value',
]);
$content = static::$client->getResponse()->getContent();
$this->assertStringContainsString('<div class="notice">The settings were changed.</div>', $content);

$settings = $this->getSettingsRepo()->findAll();
$this->assertCount(1, $settings);

$setting = $settings[0];
/* @var $setting SettingInterface */
$setting = $this->getSettingsRepo()->findOneBy([]);
$this->assertSame('name', $setting->getName());
$this->assertSame('new-value', $setting->getValue());
$this->assertSame('section', $setting->getSection());
Expand All @@ -104,9 +127,8 @@ public function testModifyAction_changeValue_cacheUsage($platform, $config, $req

$crawler = static::$client->request('GET', $this->url('craue_config_settings_modify'));
$form = $crawler->selectButton('apply')->form();
static::$client->followRedirects();
static::$client->submit($form, [
'craue_config_modifySettings[settings][0][value]' => 'new-value1',
'craue_config_modifySettings[settings][name1][value]' => 'new-value1',
]);

$this->assertTrue($cache->has('name1'));
Expand Down Expand Up @@ -136,6 +158,35 @@ public function dataModifyAction_changeValue_cacheUsage() {
return $testData;
}

/**
* Ensure that values are assigned to their originating setting when adding a setting between rendering and submitting the form.
*
* @dataProvider getPlatformConfigs
*/
public function testModifyAction_changeValue_concurrentlyAddedSetting($platform, $config, $requiredExtension) : void {
$this->initClient($requiredExtension, ['environment' => $platform, 'config' => $config]);
$this->persistSetting(Setting::create('name1', 'old-value1'));
$this->persistSetting(Setting::create('name2', 'old-value2'));
$this->persistSetting(Setting::create('name3', 'old-value3'));

$crawler = static::$client->request('GET', $this->url('craue_config_settings_modify'));
$form = $crawler->selectButton('apply')->form();

// add a new setting which would be placed between name1 and name2
$this->persistSetting(Setting::create('name11', 'old-value11'));

static::$client->submit($form, [
'craue_config_modifySettings[settings][name1][value]' => 'new-value1',
'craue_config_modifySettings[settings][name2][value]' => '',
'craue_config_modifySettings[settings][name3][value]' => 'new-value3',
]);

$this->assertSame('new-value1', $this->getSettingsRepo()->findOneBy(['name' => 'name1'])->getValue());
$this->assertSame('old-value11', $this->getSettingsRepo()->findOneBy(['name' => 'name11'])->getValue());
$this->assertNull($this->getSettingsRepo()->findOneBy(['name' => 'name2'])->getValue());
$this->assertSame('new-value3', $this->getSettingsRepo()->findOneBy(['name' => 'name3'])->getValue());
}

/**
* Ensure that an invalid form submission is handled properly.
*
Expand All @@ -147,7 +198,7 @@ public function testModifyAction_formInvalid($platform, $config, $requiredExtens

$crawler = static::$client->request('GET', $this->url('craue_config_settings_modify'));
$form = $crawler->selectButton('apply')->form();
$form->remove('craue_config_modifySettings[settings][0][value]');
$form->remove('craue_config_modifySettings[settings][name][value]');
static::$client->followRedirects();
static::$client->submit($form);
$content = static::$client->getResponse()->getContent();
Expand All @@ -167,7 +218,7 @@ public function testModifyAction_properTranslations($platform, $config, $require
static::$client->request('GET', $this->url('craue_config_settings_modify'));
$content = static::$client->getResponse()->getContent();
$this->assertStringContainsString('<legend>section no. 1</legend>', $content);
$this->assertStringContainsString('<label for="craue_config_modifySettings_settings_0_value">setting no. 1</label>', $content);
$this->assertStringContainsString('<label for="craue_config_modifySettings_settings_setting-number-one_value">setting no. 1</label>', $content);

$profile = static::$client->getProfile();
$this->assertSame(0, $profile->getCollector('translation')->getCountMissings());
Expand All @@ -186,9 +237,9 @@ public function testModifyAction_sectionOrder_defaultOrder($platform, $config, $
$content = static::$client->getResponse()->getContent();
$this->assertStringContainsString('<legend>section1</legend>', $content);
$this->assertStringContainsString('<legend>section2</legend>', $content);
$strPosField1 = strpos($content, '<label for="craue_config_modifySettings_settings_0_value">name1</label>');
$strPosField2 = strpos($content, '<label for="craue_config_modifySettings_settings_1_value">name2</label>');
$strPosField3 = strpos($content, '<label for="craue_config_modifySettings_settings_2_value">name3</label>');
$strPosField1 = strpos($content, '<label for="craue_config_modifySettings_settings_name1_value">name1</label>');
$strPosField2 = strpos($content, '<label for="craue_config_modifySettings_settings_name2_value">name2</label>');
$strPosField3 = strpos($content, '<label for="craue_config_modifySettings_settings_name3_value">name3</label>');
$this->assertTrue($strPosField2 < $strPosField1 && $strPosField1 < $strPosField3, 'The sections are rendered in wrong order.');
}

Expand All @@ -205,9 +256,9 @@ public function testModifyAction_sectionOrder_customOrder($platform, $config, $r
$content = static::$client->getResponse()->getContent();
$this->assertStringContainsString('<legend>section1</legend>', $content);
$this->assertStringContainsString('<legend>section2</legend>', $content);
$strPosField1 = strpos($content, '<label for="craue_config_modifySettings_settings_0_value">name1</label>');
$strPosField2 = strpos($content, '<label for="craue_config_modifySettings_settings_1_value">name2</label>');
$strPosField3 = strpos($content, '<label for="craue_config_modifySettings_settings_2_value">name3</label>');
$strPosField1 = strpos($content, '<label for="craue_config_modifySettings_settings_name1_value">name1</label>');
$strPosField2 = strpos($content, '<label for="craue_config_modifySettings_settings_name2_value">name2</label>');
$strPosField3 = strpos($content, '<label for="craue_config_modifySettings_settings_name3_value">name3</label>');
$this->assertTrue($strPosField2 < $strPosField3 && $strPosField3 < $strPosField1, 'The sections are rendered in wrong order.');
}

Expand Down Expand Up @@ -250,18 +301,15 @@ public function testModifyAction_customEntity($platform, $config, $requiredExten

$crawler = static::$client->request('GET', $this->url('craue_config_settings_modify'));
$content = static::$client->getResponse()->getContent();
$this->assertStringContainsString('<textarea id="craue_config_modifySettings_settings_0_value" name="craue_config_modifySettings[settings][0][value]">value</textarea>', $content);
$this->assertStringContainsString('<textarea id="craue_config_modifySettings_settings_name_value" name="craue_config_modifySettings[settings][name][value]">value</textarea>', $content);

$form = $crawler->selectButton('apply')->form();
static::$client->followRedirects();
static::$client->submit($form, [
'craue_config_modifySettings[settings][0][value]' => $newValue,
'craue_config_modifySettings[settings][name][value]' => $newValue,
]);

$settings = $this->getSettingsRepo()->findAll();
$this->assertCount(1, $settings);

$setting = $settings[0];
/* @var $setting CustomSetting */
$setting = $this->getSettingsRepo()->findOneBy([]);
$this->assertSame('name', $setting->getName());
$this->assertSame(strlen($newValue), strlen($setting->getValue()));
$this->assertSame($newValue, $setting->getValue());
Expand All @@ -275,4 +323,33 @@ public function dataModifyAction_customEntity() {
], 'config_customEntity.yml');
}

/**
* Ensure that submitting a disabled form field will keep the setting's old value.
*
* @dataProvider dataModifyAction_customEntity_disabled
*/
public function testModifyAction_customEntity_disabled($platform, $config, $requiredExtension, $environment) {
$this->initClient($requiredExtension, ['environment' => $environment . '_' . $platform, 'config' => $config]);
$this->persistSetting(CanBeDisabledSetting::create('name', 'old-value', null, true));

$crawler = static::$client->request('GET', $this->url('craue_config_settings_modify'));
$form = $crawler->selectButton('apply')->form();

$this->assertTrue($form->get('craue_config_modifySettings[settings][name][value]')->isDisabled());

static::$client->submit($form, [
'craue_config_modifySettings[settings][name][value]' => 'new-value', // will be ignored
]);

/* @var $setting CanBeDisabledSetting */
$setting = $this->getSettingsRepo()->findOneBy([]);
$this->assertSame('old-value', $setting->getValue());
}

public function dataModifyAction_customEntity_disabled() {
return self::duplicateTestDataForEachPlatform([
['config_customEntity_canBeDisabled'],
], 'config_customEntity_canBeDisabled.yml');
}

}
42 changes: 42 additions & 0 deletions Tests/IntegrationTestBundle/Entity/CanBeDisabledSetting.php
@@ -0,0 +1,42 @@
<?php

namespace Craue\ConfigBundle\Tests\IntegrationTestBundle\Entity;

use Craue\ConfigBundle\Entity\BaseSetting;

/**
* @author Christian Raue <christian.raue@gmail.com>
* @copyright 2011-2023 Christian Raue
* @license http://opensource.org/licenses/mit-license.php MIT License
*/
class CanBeDisabledSetting extends BaseSetting {

/**
* @var bool
*/
private $disabled = false;

public function setDisabled(bool $disabled) : void {
$this->disabled = $disabled;
}

public function isDisabled() : bool {
return $this->disabled;
}

/**
* Creates a {@code CanBeDisabledSetting}.
* @param string $name
* @param string|null $value
* @param string|null $section
* @param bool $disabled
* @return CanBeDisabledSetting
*/
public static function create($name, $value = null, $section = null, $disabled = false) {
$setting = parent::create($name, $value, $section);
$setting->setDisabled($disabled);

return $setting;
}

}
@@ -0,0 +1,35 @@
<?php

namespace Craue\ConfigBundle\Tests\IntegrationTestBundle\Form\Extension;

use Craue\ConfigBundle\Entity\SettingInterface;
use Craue\ConfigBundle\Form\Type\SettingType;
use Symfony\Component\Form\AbstractTypeExtension;
use Symfony\Component\Form\FormBuilderInterface;

/**
* @author Christian Raue <christian.raue@gmail.com>
* @copyright 2011-2023 Christian Raue
* @license http://opensource.org/licenses/mit-license.php MIT License
*/
class SettingTypeExtension extends AbstractTypeExtension {

/**
* {@inheritDoc}
*/
public function buildForm(FormBuilderInterface $builder, array $options) : void {
/* @var $formData SettingInterface */
$formData = $options['data'];

$config = $builder->get('value')->getForm()->getConfig();

$builder->add($config->getName(), get_class($config->getType()->getInnerType()), array_merge($config->getOptions(), [
'disabled' => $formData->isDisabled(),
]));
}

public static function getExtendedTypes() : iterable {
return [SettingType::class];
}

}

0 comments on commit bdc253e

Please sign in to comment.