Contrib.social

Detect invalid `copyFormValuesToConfig()` implementations and provide helpful error messages

Open on Drupal.org →
Created on 29 August 2023, about 1 year ago
Updated 7 November 2023, 11 months ago

Problem/Motivation

Postponed on ✨ Add optional validation constraint support to ConfigFormBase Fixed .

Based on @catch proposal at #3364506-88: Add optional validation constraint support to ConfigFormBase → , which @catch and I preferred to handle in a follow-up because it A) there is no established pattern in core yet for this DX improvement, B) there's a lot of bikeshedding potential 🤓

@catch indicated in Drupal Slack that he's worried about contrib modules adopting this and providing bogus implementations.

For mapConfigKeyToFormElementName() that's not a major concern because:

  1. the string return type forces something sensible to be returned
  2. worst case the empty string would just mean the validation error would be associated with the entire form rather than a specific form element. That's the same problem as in HEAD.

For copyFormValuesToConfig(), it's a bigger concern because the given Config object must be updated. Fortunately, we can quite easily detect the case that it's a bogus implementation: if none of the edited config objects are in fact being updated!

So, added detection logic for that, which will now throw a \LogicException to nudge developers in the right direction. For forms editing a single config it'll generate an exception like

LogicException: Drupal\update\UpdateSettingsForm:: copyFormValuesToConfig() is invalid because it did not update the edited Config object 'update.settings'. in Drupal\Core\Form\ConfigFormBase->validateForm() (line 146 of core/lib/Drupal/Core/Form/ConfigFormBase.php).

and for multiple:

LogicException: Drupal\mymodule/MySettingsForm:: copyFormValuesToConfig() is invalid because it did not update any of the edited Config objects: 'mymodule.settings', 'mymodule.moresettings'. in Drupal\Core\Form\ConfigFormBase->validateForm() (line 149 of core/lib/Drupal/Core/Form/ConfigFormBase.php).

Steps to reproduce

  1. On 11.x, do:
    diff --git a/core/modules/update/src/UpdateSettingsForm.php b/core/modules/update/src/UpdateSettingsForm.php
index 80c726dc67..784d60f444 100644
--- a/core/modules/update/src/UpdateSettingsForm.php
+++ b/core/modules/update/src/UpdateSettingsForm.php
@@ -86,6 +86,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    * {@inheritdoc}
    */
   protected static function copyFormValuesToConfig(Config $config, FormStateInterface $form_state): void {
+    return;
     switch ($config->getName()) {
       case 'update.settings':
         $config
  2. Now navigate to /admin/reports/updates/settings and submit it

Proposed resolution

Introduce either asserts or exceptions. Exceptions are preferred by Wim because they would be seen by every developer, including the ones developing with PHP assertions turned off (which is not recommended but of course supported).

Remaining tasks

TBD

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

TBD

📌 Task
Status

Closed: outdated

Version

11.0 🔥

Component
Configuration  →

Last updated 2 days ago

Created by

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024