diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 86a6d66..b87e7b4 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -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' diff --git a/Controller/SettingsController.php b/Controller/SettingsController.php index 4f18c6b..3f0b159 100644 --- a/Controller/SettingsController.php +++ b/Controller/SettingsController.php @@ -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()); } } diff --git a/Form/ModifySettingsForm.php b/Form/ModifySettingsForm.php index 0cd42e1..63d2539 100644 --- a/Form/ModifySettingsForm.php +++ b/Form/ModifySettingsForm.php @@ -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; /** @@ -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); } /** diff --git a/Form/Type/SettingType.php b/Form/Type/SettingType.php index 9593b33..1bc3518 100644 --- a/Form/Type/SettingType.php +++ b/Form/Type/SettingType.php @@ -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; @@ -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); + }); } /** diff --git a/Tests/Controller/SettingsControllerIntegrationTest.php b/Tests/Controller/SettingsControllerIntegrationTest.php index 4e29908..60f234a 100644 --- a/Tests/Controller/SettingsControllerIntegrationTest.php +++ b/Tests/Controller/SettingsControllerIntegrationTest.php @@ -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; @@ -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('/
/', $content); - $this->assertStringContainsString('', $content); - $this->assertStringContainsString('', $content); + $this->assertStringContainsString('', $content); + $this->assertStringContainsString('', $content); $this->assertStringContainsString('', $content); $form = $crawler->selectButton('apply')->form(); @@ -49,15 +51,38 @@ public function testModifyAction_noChanges($platform, $config, $requiredExtensio $content = static::$client->getResponse()->getContent(); $this->assertStringContainsString('
The settings were changed.
', $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. * @@ -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('
The settings were changed.
', $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()); @@ -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')); @@ -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. * @@ -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(); @@ -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('section no. 1', $content); - $this->assertStringContainsString('', $content); + $this->assertStringContainsString('', $content); $profile = static::$client->getProfile(); $this->assertSame(0, $profile->getCollector('translation')->getCountMissings()); @@ -186,9 +237,9 @@ public function testModifyAction_sectionOrder_defaultOrder($platform, $config, $ $content = static::$client->getResponse()->getContent(); $this->assertStringContainsString('section1', $content); $this->assertStringContainsString('section2', $content); - $strPosField1 = strpos($content, ''); - $strPosField2 = strpos($content, ''); - $strPosField3 = strpos($content, ''); + $strPosField1 = strpos($content, ''); + $strPosField2 = strpos($content, ''); + $strPosField3 = strpos($content, ''); $this->assertTrue($strPosField2 < $strPosField1 && $strPosField1 < $strPosField3, 'The sections are rendered in wrong order.'); } @@ -205,9 +256,9 @@ public function testModifyAction_sectionOrder_customOrder($platform, $config, $r $content = static::$client->getResponse()->getContent(); $this->assertStringContainsString('section1', $content); $this->assertStringContainsString('section2', $content); - $strPosField1 = strpos($content, ''); - $strPosField2 = strpos($content, ''); - $strPosField3 = strpos($content, ''); + $strPosField1 = strpos($content, ''); + $strPosField2 = strpos($content, ''); + $strPosField3 = strpos($content, ''); $this->assertTrue($strPosField2 < $strPosField3 && $strPosField3 < $strPosField1, 'The sections are rendered in wrong order.'); } @@ -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('', $content); + $this->assertStringContainsString('', $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()); @@ -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'); + } + } diff --git a/Tests/IntegrationTestBundle/Entity/CanBeDisabledSetting.php b/Tests/IntegrationTestBundle/Entity/CanBeDisabledSetting.php new file mode 100644 index 0000000..0e7954d --- /dev/null +++ b/Tests/IntegrationTestBundle/Entity/CanBeDisabledSetting.php @@ -0,0 +1,42 @@ + + * @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; + } + +} diff --git a/Tests/IntegrationTestBundle/Form/Extension/SettingTypeExtension.php b/Tests/IntegrationTestBundle/Form/Extension/SettingTypeExtension.php new file mode 100644 index 0000000..7dff68f --- /dev/null +++ b/Tests/IntegrationTestBundle/Form/Extension/SettingTypeExtension.php @@ -0,0 +1,35 @@ + + * @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]; + } + +} diff --git a/Tests/IntegrationTestBundle/Resources/config/doctrine/CanBeDisabledSetting.orm.xml b/Tests/IntegrationTestBundle/Resources/config/doctrine/CanBeDisabledSetting.orm.xml new file mode 100644 index 0000000..5360b9c --- /dev/null +++ b/Tests/IntegrationTestBundle/Resources/config/doctrine/CanBeDisabledSetting.orm.xml @@ -0,0 +1,15 @@ + + + + + + + + diff --git a/Tests/config/config_customEntity_canBeDisabled.yml b/Tests/config/config_customEntity_canBeDisabled.yml new file mode 100644 index 0000000..e8ff0b0 --- /dev/null +++ b/Tests/config/config_customEntity_canBeDisabled.yml @@ -0,0 +1,9 @@ +imports: + - { resource: config.yml } + +craue_config: + entity_name: Craue\ConfigBundle\Tests\IntegrationTestBundle\Entity\CanBeDisabledSetting + +services: + Craue\ConfigBundle\Tests\IntegrationTestBundle\Form\Extension\SettingTypeExtension: + tags: [form.type_extension] diff --git a/composer.json b/composer.json index 4efba87..d402949 100644 --- a/composer.json +++ b/composer.json @@ -55,7 +55,7 @@ }, "extra": { "branch-alias": { - "dev-master": "2.7.x-dev" + "dev-master": "3.0.x-dev" }, "symfony": { "require": "~4.4|~5.4|^6.3"