Remove visibility__active_tab from block.schema.yml

git apply ~/patches/remove-block-visibility__active_tab-from-cmi-2035725-3.patch
error: patch failed: core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php:170
error: core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php: patch does not apply
error: core/modules/block/tests/config/block.block.stark.test_block.yml: No such file or directory

Attaching updated patch.

Re-rolled after a git pull.

I'm not sure as what makes the patch fail. It applies cleanly in my local :(

Works for me.

+++ b/core/modules/block/lib/Drupal/block/Entity/Block.php
@@ -150,6 +150,9 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
+    if (isset($this->visibility['visibility__active_tab'])) {
+      unset($this->visibility['visibility__active_tab']);
+    }

This is added via form_process_vertical_tabs, and this could happen anywhere. We need a better solution outside the Block entity.

Though if it is block specific, it is BlockFormController's fault, and could be fixed in BlockFormController::submit().

NodeTypeFormController has a similar unset() for its vertical tabs.

1. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -318,6 +318,12 @@ public function submit(array $form, array &$form_state) {
   +    // visibility__active_tab is Form API state and not configuration.
   

   This should have an @todo similar to NodeTypeFormController

2. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -318,6 +318,12 @@ public function submit(array $form, array &$form_state) {
   +    if (isset($settings['values']['visibility']['visibility__active_tab'])) {
   +      unset($settings['values']['visibility']['visibility__active_tab']);
   +    }
   

   You don't need an isset. unset will work just fine by itself, no notices.

+++ b/core/modules/config_translation/config_translation.module
@@ -159,7 +159,7 @@ function config_translation_config_translation_type_info_alter(&$definitions) {
-  $definitions['date_format']['form_element_class'] = '\Drupal\config_translation\FormElement\DateFormat';
+  $definitions['date_format']['form_element_class'] = '\Drupal\coBlockFormControllernfig_translation\FormElement\DateFormat';

Somehow this snuck in?

Otherwise looks good.

Thanks!

Shouldn't this be added to https://api.drupal.org/api/drupal/includes!form.inc/function/form_state_... instead?

What catch says in #23 makes sense to me.

Thanks, this is the correct fix.

I looked into this a bit because I wondered why this problem does not occur with node types, who also have a vertical tab on the edit form. So I followed the code flow for config entity submit functions.

For both node types as well as blocks the __active_tab information actually makes it into the $entity due to buildEntity() foreach-ing over the the form values and sticking them in there. With node types, however, the vertical tab is in the top-level of the form structure so that it ends up as a top-level property of the entity object. But since that key is not in getExportProperties() it never gets saved into the config.

With block.module, however, the vertical is inside the 'visibility' form key. And 'visibility' is a key in getExportProperties() because it contains needed information. That's why it eventually gets saved into the config.

The correct fix for this is removing the key from the entity itself. We do not want to manipulate the form state, as that is by reference.

For node types this should be done in theory as well, but that's out of scope here and less problematic because it doesn't get saved.

Note: The get() set() business in the patch is needed because 'visibility' is a protected property.

Re-roll.

Looks good!

Very nice catch!

Committed and pushed to 8.x. Thanks!

Moving back to 7.x for the backport.

Opened #2190895: Revamp vertical tabs to not save form values.