Revamp vertical tabs to not save form values

+1, this is probably a better approach. :)

Not to sound ignorant, but why would we ever want to iterate over form values and save those? We should be iterating over what we know can be saved, and then get the values from the submitted form data instead, so things don't break like they do here, or when people alter forms to add new elements.

Closed these related issues as duplicates of this one.

Forgot tag

This is a patch (for D7) which adds $form_state['values_clean']. Any values added to that array will be unset by form_state_values_clean(). Seems like a clean and intuitive way to do it instead of creating another element just for vertical tabs. I wonder if the button logic could be greatly simplified as well in form_state_values_clean() using this technique? Looking for feedback on this approach before I port to D8.

To be clearer about the issue, the results of this bug is that **__active_tab variables show up in the variables tables and can cause features that export them to be considered overridden unnecessarily. They aren't even used when you return to a page or anything. This most likely happens on settings_form forms since they blindly save all values as variables. (< @Xano this is why it's an issue.)

Until the version field is switched to 7.x, this patch will fail testing. Hiding the file for now, 8.0.x will need to be fixed first regardless.

Roger, porting the D7 patch to D8 now while I have it in my head.

Here is the D8 patch which does things very similarly. I searched around for the special code that was added in #2035725: Remove visibility__active_tab from block.schema.yml but it looks like it was refactored out in commit 1228438e65d [1228438e65d].

Oops, can use 'string' for php type-hinting.

Awesome, this looks good to me. Just setting to CNW since this will also need additional tests (may already have some similar existing ones somewhere).

1. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -197,6 +197,24 @@ class FormState implements FormStateInterface {
   +  protected $clean_values = array(
   
   @@ -974,6 +992,30 @@ public function setValueForElement(array $element, $value) {
   +    return $this->clean_values;
   

   Some of the old property names are snake_case due to since-removed ArrayAccess, but all new properties should be lowerCamelCase

2. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -974,6 +992,30 @@ public function setValueForElement(array $element, $value) {
   +  public function getCleanValues() {
   ...
   +  public function setCleanValues(array $clean_values) {
   ...
   +  public function addCleanValue($clean_value) {
   

   These aren't the values themselves, they're the keys.

3. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -1164,12 +1206,9 @@ public function getFormObject() {
   +    foreach($this->getCleanValues() as $value) {
   

   missing a space

Cool, here it is with tests and tim's suggested fixes. Names now are like addCleanValueKey() instead of addCleanValue().

I assume we should add the new methods to FormStateInterface as well? Here is a patch that adds that. FYI, I think this would actually be my first core-commit ..kinda long overdue :P

More functional tests, including for vertical tabs itself.

1. +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
   @@ -1046,4 +1046,30 @@ public function isValidationComplete();
   +   * @return string $key
   

   I think this should be
   @param string $key

2. +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
   @@ -1046,4 +1046,30 @@ public function isValidationComplete();
   +   *   The key if the form value to be cleaned.
   

   Suggestion: The form value key to be cleaned.

This is really close!

1. +++ b/core/lib/Drupal/Core/Form/FormState.php
   @@ -197,6 +197,24 @@ class FormState implements FormStateInterface {
   +  protected $cleanValueKeys = array(
   
   +++ b/core/modules/system/src/Tests/Form/ElementsVerticalTabsTest.php
   @@ -66,4 +67,12 @@ function testDefaultTab() {
   +    $values = Json::decode($this->drupalPostForm('form_test/form-state-values-clean', array(), t('Submit')));
   +    $this->assertFalse(isset($values['vertical_tabs__active_tab']), format_string('%element was removed.', array('%element' => 'vertical_tabs__active_tab')));
   

   Here, and elsewhere, please use [] instead of array()

2. +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
   @@ -1046,4 +1046,30 @@ public function isValidationComplete();
   +  public function setCleanValueKeys(array $values);
   ...
   +  public function getCleanValueKeys();
   

   Might as well flip the order of these, to match the order in FormState.

3. +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
   @@ -1046,4 +1046,30 @@ public function isValidationComplete();
   +   * @return array $keys
   

   @return array

4. +++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
   @@ -1046,4 +1046,30 @@ public function isValidationComplete();
   +   * @return string $key
   

   @param string $key

   @return $this

5. +++ b/core/modules/system/src/Tests/Form/StateValuesCleanTest.php
   @@ -49,6 +49,9 @@ function testFormStateValuesClean() {
   +    $this->assertFalse(isset($values['wine']), format_string('%element was removed.', array('%element' => 'wine')));
   

   Here, and elsewhere, use String::format() instead of format_string()

6. +++ b/core/tests/Drupal/Tests/Core/Form/FormStateTest.php
   @@ -530,6 +530,23 @@ public function testTemporaryValue() {
   +  /**
   +  * @covers ::getCleanValues
   +  * @covers ::setCleanValues
   +  * @covers ::addCleanValue
   +  * @covers ::cleanValues
   +  */
   +  public function testCleanValues() {
   

   Please split these into four test methods, one for each @covers

   Consider using @depends to build from one method to the next.

7. +++ b/core/tests/Drupal/Tests/Core/Form/FormStateTest.php
   @@ -530,6 +530,23 @@ public function testTemporaryValue() {
   +    $form_state = New FormState();
   

   new FormState()

Also, please include interdiffs when making changes between patches. Thanks!

I believe the feedback in #22 means this issue needs work :)

Huh, I thought I'd updated this :/ I have all the fixes and just needed to upload the diffs I think.. @todo

Here are the files w/ the interdiff

Sorry, I wasn't specific about the String::format thing, fixing for you just to get this done.

As that's my only contribution here, I feel comfortable RTBCing this.

Thanks @frankcarey!

Minor nit pick, but shouldn't the manual unset that was introduced in #2035725: Remove visibility__active_tab from block.schema.yml be removed here since it's already removed from the array by this patch?

This patch:

+++ b/core/lib/Drupal/Core/Render/Element/VerticalTabs.php
@@ -100,6 +100,9 @@ public static function processVerticalTabs(&$element, FormStateInterface $form_s
+    // Clean up the active tab value so it's not accidentally stored in
+    // settings forms.
+    $form_state->addCleanValueKey($name . '__active_tab');

From other issue (which has the @todo this patch fixes):

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
@@ -359,6 +360,21 @@ public function delete(array $form, array &$form_state) {
+  public function buildEntity(array $form, array &$form_state) {
+    $entity = parent::buildEntity($form, $form_state);
+
+    // visibility__active_tab is Form API state and not configuration.
+    // @todo Fix vertical tabs.
+    $visibility = $entity->get('visibility');
+    unset($visibility['visibility__active_tab']);
+    $entity->set('visibility', $visibility);
+
+    return $entity;
+  }

That hack was removed in #2278541: Refactor block visibility to use condition plugins.

This is a bug.

Yay, the automated @covers checker works!

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 1218fdf and pushed to 8.0.x. Thanks!

Awesome :)

@frankcarey, I've re-queued #7, but figured you may want to revisit the 7.x patch so it's a little more inline with the final patch that got into 8.x?

Re uploading the same D7 patch to try to get tests running for it again (old one doesn't seem to work anymore)