Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Installing localgov_forms when webforms already installed overwrites webform settings #34

Open
CATCHPOLEBHC opened this issue Mar 3, 2023 · 7 comments

Comments

@CATCHPOLEBHC
Copy link

Having recently installed localgov forms, discovered that webform settings were overwritten. Could this be amended to check the is_syncing flag and so avoid overwriting current setting?

@andybroomfield
Copy link
Contributor

This came up at BHCC when we tried to install localgov_forms within a site that already had webforms enabled and our settings got overwriten with the config from config/install/localgov_forms.webform.settings.yml in hook_install.

We can get around this behaviour by commenting that out on initial install, or pathching the module.
Ideally if webform settings already exist, this module shouldn't replace them, though I'm unsure how to do that given the intial install will set up webform.settings.yml. At a minimum however there should be a hook_install($is_syncing) flag added, and the install config should be placed behind a if (!$is_syncing) { conditional. This would allow those who have there own webform configuration avoid getting it replaced during config sync operations, and we can do patching during initial development.

Ideally, we should move these settings to a submodule, labelled Localgov_forms_ideal_settings or something similar, so existing webform enviornment is left alone unless the submodule is enabled.

@ekes
Copy link
Member

ekes commented Mar 3, 2023

If you move them to config/optional?

@andybroomfield
Copy link
Contributor

Currently this is executed during module install

function localgov_forms_install() {

  // Override Webforms default configuration.
  $config = \Drupal::configFactory()->getEditable('localgov_forms.webform.settings');
  $settings = \Drupal::configFactory()->getEditable('webform.settings');
  $settings->setData($config->get())->save();
}

@ekes
Copy link
Member

ekes commented Mar 3, 2023

Yes I think this might cause multiple issues.

What are the differences from the default?
I guess it's actually only certain fields that should be set to LGD/GDS compatible best practice or requirements? Others could just be left alone.
Could there be some sort of 'reset' to LGD defaults button?

@finnlewis
Copy link
Member

finnlewis commented Dec 4, 2023

@Adnan-cds mentions this as an issue to resolve before a stable release.

First step is to remove the default config from the install hook.

As a follow up , we might want to review the default config and make a more intelligent default config if we are installing webform for the first time, but not if it is already installed.

See https://github.com/localgovdrupal/localgov_forms/blob/1.x/config/install/localgov_forms.webform.settings.yml

Question: how different is that config from Webform's default?

@Adnan-cds
Copy link
Contributor

Adnan-cds commented Dec 15, 2023

Suggested changes for the default Webform settings:

  • Add the "Webform" menu item in the admin menu. This settings is in the "Advanced" tab of "Configuration" tab (/admin/structure/webform/config/advanced).
  • Enabled following Webform elements: File, Value, Name, Email confirm

@finnlewis
Copy link
Member

Discussing in Merge Monday, sounds like a good starting point!

Note that this is just the starting point, developers are likely to go on and configure Webform as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants