Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
visibility__active_tab is defined in block.schema.yml and exported in every block yml file. This property stores the last active tab from Block Visibility settings. I think this data should not stored in CMI. Its used for form validation only ...
Comment | File | Size | Author |
---|---|---|---|
#32 | drupal_2035725_32.patch | 8.39 KB | Xano |
Comments
Comment #1
webflo CreditAttribution: webflo commentedComment #3
webflo CreditAttribution: webflo commentedWrapped this code in isset() because not all blocks have visibility settings.
Comment #4
benjy CreditAttribution: benjy commentedComment #5
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedAttaching updated patch.
Comment #7
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedRe-rolled after a git pull.
Comment #9
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedI'm not sure as what makes the patch fail. It applies cleanly in my local :(
Comment #10
webflo CreditAttribution: webflo commentedI think the core yaml files have changed.
Comment #11
webflo CreditAttribution: webflo commentedgo testbot!
Comment #12
longwaveWorks for me.
Comment #13
tim.plunkettThis 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().
Comment #14
longwaveNodeTypeFormController has a similar unset() for its vertical tabs.
Comment #15
webflo CreditAttribution: webflo commentedOk. Move this logic to BlockFormController::submit(). I remove the array key in $settings because i dont want to mess with the form state.
Comment #16
tim.plunkettThis should have an @todo similar to NodeTypeFormController
You don't need an isset. unset will work just fine by itself, no notices.
Comment #17
webflo CreditAttribution: webflo commentedComment #18
tim.plunkettSomehow this snuck in?
Otherwise looks good.
Comment #19
webflo CreditAttribution: webflo commentedJup. Sorry.
Comment #20
tim.plunkettThanks!
Comment #23
catchShouldn't this be added to https://api.drupal.org/api/drupal/includes!form.inc/function/form_state_... instead?
Comment #24
benjy CreditAttribution: benjy commentedWhat catch says in #23 makes sense to me.
Comment #25
webflo CreditAttribution: webflo commentedOk moved the code to form_state_values_clean.
Comment #26
webflo CreditAttribution: webflo commentedRemoved this whitespace change.
Comment #27
webflo CreditAttribution: webflo commentedComment #28
webflo CreditAttribution: webflo commentedI think the patch from comment #19 (RTBC by tim.plunkett) was better. Because there is no generic way to detect where the active tab is stores in $form_state['values']. The active tab state depends on the form structure.
Comment #29
webflo CreditAttribution: webflo commentedComment #30
webflo CreditAttribution: webflo commentedOk, lets move the vertical tab clenaup to buildEnitty.
Comment #31
tstoecklerThanks, 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.
Comment #32
XanoRe-roll.
Comment #33
tstoecklerLooks good!
Comment #34
webchickVery nice catch!
Committed and pushed to 8.x. Thanks!
Moving back to 7.x for the backport.
Comment #35
tstoecklerOpened #2190895: Revamp vertical tabs to not save form values.