Setting #access = FALSE on a vertical tab does not prevent input processing of the contained form elements

Ok, identified the problem and I also have a patch that fixes the tests.

This is not specific to checkboxes in any way. The problem is that setting #access on a vertical tab is not passed through to the fieldsets that are displayed within that vertical tab. Which makes totally sense, because they are, at that point (in form_builder()) not children of the vertical tab. They're only moved around later.

The result is that denying access does prevent rendering of those fieldsets but it does not prevent that the form elements within them go through input processing. Only now the difference between checkboxes and textfields shows because FAPI can not differentiate between not submitted and not checked checkboxes, so it assumes unchecked. While it can differentiate for textfields where it uses, in that case, the default value.

What the patch does is check for a #group during form_builder() and if there is one, check the #access of that. The result is that this works as expected by the test.

IMHO, the #access behavior of a vertical_tab is completely wrong. It is a visual thing, setting #access => FALSE on it should not prevent the fieldset from being rendered, it should only prevent them from being rendered as a vertical tab. Why should $form['vertical_tab']['#access'] affect $form['tab1']['#access'], this is not like anything else in FAPI works. That the fieldsets are hidden is more a side effect of them being moved around in process functions.

However, the problem is that it currently looks as if #access would work correctly, which probably means that we have to make it work the way it seems to do. For security reasons in 7.x, if nothing else.

Thanks guys this is great.

Hm, hold on a second. This isn't specific to vertical tabs (or things with #group), is it?

Basically, won't the same issue occur for any other situation where a pre-render callback moves the elements around in the form array?

If so, maybe we don't want to special-case vertical tabs here, but rather have it behave consistently. In which case, documentation may be the correct way to solve it. (As Berdir said above, it doesn't seem like an actual bug that #access changes made in the rendering phase only affect how the form is rendered, not how it's processed.)

Although I can see how this is pretty confusing (especially in the case of vertical tabs, since they're used so often and the pre-render stuff happens for you behind the scenes)...

They do. That's exactly the problem here. As a side effect of the moving things around during pre-render, the fieldsets actually aren't rendered. Otherwise there would be no reason for them to be not shown, which is what I would have expected from looking at the form structure.

Agreed that the #group check in form_builder() is not a perfect solution. Maybe we can add a recursive function that is triggered *after* the pre-render callbacks that takes care of enforcing #access recursively?

The summary states an API change as a result of this patch, so tagging accordingly.

I disagree with the problem statement, and thus also with the proposed fix.

When setting #access to FALSE on a vertical tabs element, then the only expectation one might reasonably have is that there won't be vertical tabs. That is, because the form structure is detached from rendering. That is also why vertical tabs are processed and applied during rendering.

If you want to make form input elements inaccessible, then you have to set #access on the individual elements (or their parent structure).

In turn, the only bug I could agree with is that neither form_process_fieldset() nor form_process_vertical_tabs() checks for the #access property value at all.

In turn, I think this entire issue needs to be relabeled and re-classified.

Well form_process_fieldset does not need to check for #access cos the generic #access inheritance takes care of that. I have no opinion on what #access on vertical tabs should or should not do.

Attached patch fixes the bug in the way I explained in #22.

#process is unaware of #access, so form_process_vertical_tabs() needs to be stopped from doing unholy things if the element it operates on (#type vertical_tabs) is not permitted to proceed.

In essence, this basically hints at another, different architectural issue, which doesn't seem to be the topic of this issue though. My "last known" refactoring of vertical tabs (#657668: Vertical tabs break Form API) attempted to make sure that most of the logic stays in the rendering layer and does not affect form building/processing. That does not seem to be sufficient though, as this patch proves, since the #process still runs during processing. In turn, I guess that the injection of the "new fieldset", as being performed in this #process, actually needs to be moved into a separate #pre_render, so that the rendering properly takes #access into account. Or similar. Anyway, that shouldn't be part of this issue.

Right.

If it wasn't clear, I agree with sun's opinion (that was my suggestion from the beginning, I just wanted to show how it could be fixed). I think we should keep the tests and just change the assertions a bit (make sure that the fieldsets actually are there but no vertical-tab stuff, I'm sure there are some classes or something that we can look for.

A comment wouldn't hurt either :)

I guess the patch as it stands now actually proves that we don't have any test coverage for vertical tabs (Not talking about client side JS tests, just tests that the vertical tab markup is there) because doesn't the empty() check mean that we wouldn't have any vertical tabs?

So maybe add a second vertical tab to the form and assert that that is shown?

steps to reproduce

24: drupal8.vertical-tabs-access.24.patch queued for re-testing.

Here's a re-roll for now.

I wanted to add a comment, but I'm actually no longer sure what this issue is trying to fix — even after reading comments.

I also don't know whether the proposed fix is still compatible with recent changes to #group in D8... quite a lot has changed in that field.

I think we need a proper test + actual expectations first. For D8, please take inspiration from the DUTB test in #2214055: Programmed form submission does not get a triggering_element

What happened to form_process_vertical_tabs()? Can't grep it any more.

Ok, then let's see what the bot thinks of this.

The patch in #38 is missing the test from the previous patches (most recent in #15).

Refactored the test and form from #15 combined with the patch in #38. Didn't change any logic, just made sure the code was up to date with the newest Core API.

Putting issue back in review.

Thanks @samuel.mortenson! Could you please also upload the test-only portion of this (eg, everything but the fix) to illustrate that the tests fail without the fix?

Done

Fix makes sense, and the tests look good!

@xmacinfo You queued the test-only patch for re-testing, which was meant to fail. The full patch from #40 succeeds normally, as it includes the fix. Please add a comment to the issue next time you change the status. Setting back to RTBC.

Indeed! Re-tested the wrong patch. Sorry!

No problem, just making sure we aren't missing something in the current patch. :-)

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed ce45b6e and pushed to 8.0.x. Thanks!

diff --git a/core/modules/system/src/Tests/Form/ElementsAccessTest.php b/core/modules/system/src/Tests/Form/ElementsAccessTest.php
index a0a6929..478f500 100644
--- a/core/modules/system/src/Tests/Form/ElementsAccessTest.php
+++ b/core/modules/system/src/Tests/Form/ElementsAccessTest.php
@@ -26,7 +26,7 @@ class ElementsAccessTest extends WebTestBase {
   /**
    * Ensures that child values are still processed when #access = FALSE.
    */
-  function testAccessFalse() {
+  public function testAccessFalse() {
     $this->drupalPostForm('form_test/vertical-tabs-access', NULL, t('Submit'));
     $this->assertNoText(t('This checkbox inside a vertical tab does not have its default value.'));
     $this->assertNoText(t('This textfield inside a vertical tab does not have its default value.'));
diff --git a/core/modules/system/tests/modules/form_test/src/Form/FormTestVerticalTabsAccessForm.php b/core/modules/system/tests/modules/form_test/src/Form/FormTestVerticalTabsAccessForm.php
index edf4970..0f3a2cc 100644
--- a/core/modules/system/tests/modules/form_test/src/Form/FormTestVerticalTabsAccessForm.php
+++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestVerticalTabsAccessForm.php
@@ -24,80 +24,80 @@ public function getFormId() {
    */
   public function buildForm(array $form, FormStateInterface $form_state) {
     $form['vertical_tabs1'] = array(
-        '#type' => 'vertical_tabs',
-      );
+      '#type' => 'vertical_tabs',
+    );
     $form['tab1'] = array(
-        '#type' => 'fieldset',
-        '#title' => t('Tab 1'),
-        '#collapsible' => TRUE,
-        '#group' => 'vertical_tabs1',
-      );
+      '#type' => 'fieldset',
+      '#title' => t('Tab 1'),
+      '#collapsible' => TRUE,
+      '#group' => 'vertical_tabs1',
+    );
     $form['tab1']['field1'] = array(
-        '#title' => t('Field 1'),
-        '#type' => 'checkbox',
-        '#default_value' => TRUE,
-      );
+      '#title' => t('Field 1'),
+      '#type' => 'checkbox',
+      '#default_value' => TRUE,
+    );
     $form['tab2'] = array(
-        '#type' => 'fieldset',
-        '#title' => t('Tab 2'),
-        '#collapsible' => TRUE,
-        '#group' => 'vertical_tabs1',
-      );
+      '#type' => 'fieldset',
+      '#title' => t('Tab 2'),
+      '#collapsible' => TRUE,
+      '#group' => 'vertical_tabs1',
+    );
     $form['tab2']['field2'] = array(
-        '#title' => t('Field 2'),
-        '#type' => 'textfield',
-        '#default_value' => 'field2',
-      );
+      '#title' => t('Field 2'),
+      '#type' => 'textfield',
+      '#default_value' => 'field2',
+    );
 
     $form['fieldset1'] = array(
-        '#type' => 'fieldset',
-        '#title' => t('Fieldset'),
-      );
+      '#type' => 'fieldset',
+      '#title' => t('Fieldset'),
+    );
     $form['fieldset1']['field3'] = array(
-        '#type' => 'checkbox',
-        '#title' => t('Field 3'),
-        '#default_value' => TRUE,
-      );
+      '#type' => 'checkbox',
+      '#title' => t('Field 3'),
+      '#default_value' => TRUE,
+    );
 
     $form['container'] = array(
-        '#type' => 'container',
-      );
+      '#type' => 'container',
+    );
     $form['container']['field4'] = array(
-        '#type' => 'checkbox',
-        '#title' => t('Field 4'),
-        '#default_value' => TRUE,
-      );
+      '#type' => 'checkbox',
+      '#title' => t('Field 4'),
+      '#default_value' => TRUE,
+    );
     $form['container']['subcontainer'] = array(
-        '#type' => 'container',
-      );
+      '#type' => 'container',
+    );
     $form['container']['subcontainer']['field5'] = array(
-        '#type' => 'checkbox',
-        '#title' => t('Field 5'),
-        '#default_value' => TRUE,
-      );
+      '#type' => 'checkbox',
+      '#title' => t('Field 5'),
+      '#default_value' => TRUE,
+    );
 
     $form['vertical_tabs2'] = array(
-        '#type' => 'vertical_tabs',
-      );
+      '#type' => 'vertical_tabs',
+    );
     $form['tab3'] = array(
-        '#type' => 'fieldset',
-        '#title' => t('Tab 3'),
-        '#collapsible' => TRUE,
-        '#group' => 'vertical_tabs2',
-      );
+      '#type' => 'fieldset',
+      '#title' => t('Tab 3'),
+      '#collapsible' => TRUE,
+      '#group' => 'vertical_tabs2',
+    );
     $form['tab3']['field6'] = array(
-        '#title' => t('Field 6'),
-        '#type' => 'checkbox',
-        '#default_value' => TRUE,
-      );
+      '#title' => t('Field 6'),
+      '#type' => 'checkbox',
+      '#default_value' => TRUE,
+    );
 
     $form['actions'] = array(
-        '#type' => 'actions',
-      );
+      '#type' => 'actions',
+    );
     $form['actions']['submit'] = array(
-        '#type' => 'submit',
-        '#value' => t('Submit'),
-      );
+      '#type' => 'submit',
+      '#value' => t('Submit'),
+    );
     return $form;
   }
 
@@ -132,4 +132,5 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   public function submitForm(array &$form, FormStateInterface $form_state) {
     drupal_set_message(t('The form submitted correctly.'));
   }
+
 }

All that fixed on commit.

There was something we wanted to backport here... not sure if it was the final committed D8 patch or not.